[PATCH] D125291: Introduce @llvm.threadlocal.address intrinsic to access TLS variable (1/3)

[Chuanqi Xu via Phabricator via llvm-commits]

ChuanqiXu marked an inline comment as done.
ChuanqiXu added a comment.

In D125291#3535196 <https://reviews.llvm.org/D125291#3535196>, @nhaehnle wrote:

> You can use the "Edit Related Revisions" option in the right-hand side menu of Phabricator to link this revision with the others of the series.

I forgot to reply this one. It is intended to not add related revisions. Since these revisions don't depend on each other from the perspective of codes. We could commit them in either order.

In D125291#3548671 <https://reviews.llvm.org/D125291#3548671>, @jyknight wrote:

> So, I've been spending some significant time looking into this. I found that I couldn't really review this change for correctness, because I find it basically impossible to figure out whether the intrinsic calls have actually been added to all the right places in Clang or not. (And my intuition said "not", but then couldn't tell me where it's wrong.)

Thanks for spending the time! To answer the question "Whether the intrinsic calls have actually been added to all the right places in Clang or not", I insert a verify pass in the beginning of the middle end pipeline to verify if all the uses of TLS variable is wrapped by the intrinsic. And my conclusion is NO. Not all the places is guarded by the intrinsic. Concretely, the dynamic initializer of TLS variable. See https://www.godbolt.org/z/a4bK8v67o. I am not sure if this is the RIGHT place you said.

> So, I started hacking up a prototype of a change to make the type of a TLS global variable be `token` instead of `ptr`. This allows missing calls to manifest as IR construction errors.
>
> So far the biggest missing piece that identified in this review is function-local thread_locals (although I haven't actually gotten something fully working). Sadly, the handling of function-local statics in Clang is really rather hairy, what with objc blocks and openmp support both doing things that seem rather ill-advised to me. I'm toying with some cleanups there, to see if it can be simplified a bit. I don't have a full idea, yet, what changes need to be made to this review.

Thanks for pointing this out! I also spent significant time to try to handle function-local thread locals in the last few days. And I have't found a solution yet. It is more complex indeed.

> Anyhow -- I think the prototype I'm fiddling with is also along the path to the ideal long-term state, but pushing it beyond a prototype seems like it'll be a pain in the ass due to the bitcode compatibility requirement. (The bitcode upgrader would need to know how to transform all constant expressions using a TLS global into non-constant IR instructions, starting with a call to llvm.threadlocal.address -- in every function where the "constant" is referenced. For uses outside a function body, it transforms to an arbitrary address (e.g. null), instead.)

Oh, I never heard about the IR upgrader before. This is missed indeed.

---

Summarize things up. Here are 3 problems we found:
(1) The use in the dynamic initializer of TLS variable.
(2) Function local thread locals.
(3) IR Upgrader.

>From my perspective, only the second problem (2) is the problem we must solve. The first problem looks fine to me in practice. And I am wondering if it is problem for the third problem. Since if I understand the problem correctly, it means that the newer compiler couldn't compile the IR from older compiler. And I think it wouldn't be that case after the patch landed. Do I misunderstand it?


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

https://reviews.llvm.org/D125291