Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
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)
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?
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 ...
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.