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

Thu Jun 9 03:05:45 PDT 2022

ChuanqiXu marked 4 inline comments as done.
ChuanqiXu added a comment.

It looks like there are two problems now:
(1) The use of TLS variable in the dynamic initializer and the use of generated TLS variable (`__tls_guard`)  doesn't get wrapped in @llvm.threadlocal_address() intrinsics. From my perspective, it is fine since the initializers should never be coroutines. (I meant to fix the coroutines bugs at the very start).
(2) The IR upgrader problem. It is fine to me too since we won't block the use of TLS variable directly after the patch landed (maybe we would in the longer future). So the higher version of LLVM will be able to compile the IR from older version after the patch landed. So it is not a problem to me. (It looks like the backward compatibility is not emphasized. This is the first time I saw the problem in the community)

================
Comment at: clang/lib/CodeGen/CGExpr.cpp:2609-2610
+  if (VD->getTLSKind() != VarDecl::TLS_None &&
+      // We only use @llvm.threadlocal.address if opaque pointers enabled.
+      // Otherwise, we need to pay for many unnecessary bitcasts.
+      //
----------------
jyknight wrote:
> This should be handled by using an overloaded intrinsic, so you get the entire family llvm.threadlocal.address.* with any pointer-type as the argument and the same type as the return value (that'll happen when you switch the intrinsic to use llvm_anyptr_ty).
Yeah, it could be handled by an overloaded intrinsic. But given the process of opaque pointers goes well really, I feel like it is redundant to support non opaque pointer mode. It shouldn't affect users since opaque pointers is enabled by default as far as I know.

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

https://reviews.llvm.org/D125291