[PATCH] D19054: [SystemZ] Support __builtin_thread_pointer.

[Ulrich Weigand via llvm-commits]

uweigand added a comment.

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.


================
Comment at: include/llvm/IR/IntrinsicsSystemZ.td:20
@@ -14,1 +19,3 @@
+}
+
 class SystemZUnaryConv<string name, LLVMType result, LLVMType arg>
----------------
Please move this further down after all the class/multiclass definitions, maybe into a new section "Miscellaneous intrinsics".

================
Comment at: lib/Target/SystemZ/SystemZISelLowering.cpp:3409
@@ +3408,3 @@
+    return lowerThreadPointer(DL, DAG);
+  }
+
----------------
For consistency with the coding style later on, please just do
  return lowerThreadPointer(SDLoc(Op), DAG);



Repository:
  rL LLVM

http://reviews.llvm.org/D19054