[PATCH] D125291: Introduce @llvm.threadlocal.address intrinsic to access TLS variable

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

FWIW the bitcode patch has landed, so implementing the variant using a token type should be possible now. I'm not sure whether it's better to start with the current patch where the intrinsic is optional, or go directly to the one where it is required.



================
Comment at: llvm/docs/LangRef.rst:24415
+
+      declare ptr @llvm.threadlocal.address(ptr) nounwind readnone willreturn
+
----------------
Don't we need to overload this intrinsic by return type, so it works with different address spaces?


================
Comment at: llvm/docs/LangRef.rst:24420
+
+The first argument is pointer, which refers to a thread local variable.
+
----------------
Should we enforce here (with a verifier check) that the argument is a thread-local global variable? I assume we //don't// want to allow weird things like `@llvm.threadlocal.address(c ? @g1 : @g2)` right? (Though I guess, without thread-local globals using a token type, nothing would prevent optimizations from forming this.)


================
Comment at: llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp:231
+      Changed |= lowerThreadLocalIntrinsics(F);
+      break;
     }
----------------
I don't think this belongs here, this should get dropped by SelectionDAGBuilder.


================
Comment at: llvm/test/Transforms/PreISelIntrinsicLowering/threadlocal_address.ll:1
+; RUN: opt -pre-isel-intrinsic-lowering -opaque-pointers -S -o - < %s | FileCheck %s
+
----------------
`-opaque-pointers` flag is not necessary.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125291/new/

https://reviews.llvm.org/D125291