This is an archive of the discontinued LLVM Phabricator instance.

[SystemZ] Support __builtin_thread_pointer.
ClosedPublic

Authored by koriakin on Apr 13 2016, 4:40 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

koriakin retitled this revision from to [SystemZ] Support __builtin_thread_pointer..
koriakin updated this object.
koriakin added a reviewer: uweigand.
koriakin set the repository for this revision to rL LLVM.
koriakin added a subscriber: llvm-commits.

This serves two purposes:

  • __builtin_thread_pointer can be used from clang
  • llvm.s390.thread.pointer can be used in IR, allowing us to implement proper stack protector support (aiming at TLS instead of __stack_chk_guard)
uweigand edited edge metadata.Apr 13 2016, 5:56 AM

I think this looks good as far as it goes (see minor coding style comments inline). However:

  • I don't think this in itself will make __builtin_thread_pointer work in clang, there need to be additional changes to clang (at least adding the builtin to include/clang/Basic/BuiltinsSystemZ.def, providing some test cases). Are you planning on contributing a clang patch as well?
  • It's a bit weird to define __builtin_thread_pointer as platform-specific intrinsic (on GCC it's a common intrinsic). But I see that ARM/AArch64 already have __builtin_thread_pointer as platform-specific intrinsic as well, so that may be precendent ... It would probably still be good to ask for feedback from the wider LLVM community how this should be handled.
include/llvm/IR/IntrinsicsSystemZ.td
20 ↗(On Diff #53535)

Please move this further down after all the class/multiclass definitions, maybe into a new section "Miscellaneous intrinsics".

lib/Target/SystemZ/SystemZISelLowering.cpp
3409 ↗(On Diff #53535)

For consistency with the coding style later on, please just do

return lowerThreadPointer(SDLoc(Op), DAG);

Yes, I'm preparing a clang patch as well.

As for making this target-independent: sounds like a good idea (I have a similiar patch for ppc in queue), but aarch64 already has it as target-specific - removing it would be a backwards compatibility break, right?

Yes, I'm preparing a clang patch as well.

Very good, thanks.

As for making this target-independent: sounds like a good idea (I have a similiar patch for ppc in queue), but aarch64 already has it as target-specific - removing it would be a backwards compatibility break, right?

Well, we'd probably have to continue to accept the aarch64 LLVM IR intrinsic in the back-end to avoid breaking compatibility. But that would still allow for adding a new target-independent LLVM IR intrinsic, and using that in clang common code to implement __builtin_thread_pointer for all platforms ...

koriakin edited edge metadata.

This version depends on D19098. I'll wait a bit with the clang patch, since I have thread pointer support patches lined up for a new more targets - let's add them all in one go.

uweigand accepted this revision.Apr 14 2016, 3:00 AM
uweigand edited edge metadata.

LGTM once the dependencies are in.

This revision is now accepted and ready to land.Apr 14 2016, 3:00 AM
This revision was automatically updated to reflect the committed changes.