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

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a reviewer: nikic.
ChuanqiXu marked 2 inline comments as done.
ChuanqiXu added a comment.

In D125291#3629276 <https://reviews.llvm.org/D125291#3629276>, @nikic wrote:

> 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.

I preferred to start with the current patch. Since my original idea is to fix the thread problems in coroutines and @jyknight said we could fix the two problems in one approach. But I still want to fix the thread problems in coroutines first.

---

A big problem I meet now is about the constant expression would merge into the argument automatically so the verifier would fail. Here is the example:

  C++
  union U {
    int a;
    float f;
    constexpr U() : f(0.0) {}
  };
  static thread_local U u;
  void *p = &u;

Then it would generate following code for `u`:

  define internal void @__cxx_global_var_init() #0 section ".text.startup" {
  entry:
    %0 = call %union.U* @llvm.threadlocal.address.p0s_union.Us(%union.U* bitcast ({ float }* @_ZL1u to %union.U*))
    %1 = bitcast %union.U* %0 to i8*
    store i8* %1, i8** @p, align 8
    ret void
  }

Then the verifier would complain since the argument of `@llvm.threadlocal.address` is not theadlocal global value. And I feel this might not be a simple problem to fix. I feel like we could only fix it after we made https://discourse.llvm.org/t/rfc-remove-most-constant-expressions/63179. But it should be a long term goal to me.

---

So now it looks like there are two options:

1. Remove the verifier part.
2. Wait until we removed most constant expressions. In this case, I could only to pick up original methods to solve the thread problems in coroutines.

@jyknight @nikic  @nhaehnle @efriedma @rjmccall How do you think about this?



================
Comment at: llvm/docs/LangRef.rst:24415
+
+      declare ptr @llvm.threadlocal.address(ptr) nounwind readnone willreturn
+
----------------
nikic wrote:
> Don't we need to overload this intrinsic by return type, so it works with different address spaces?
Done. So we could remove the limit for opaque pointers too. So now it covers more tests.


================
Comment at: llvm/docs/LangRef.rst:24420
+
+The first argument is pointer, which refers to a thread local variable.
+
----------------
nikic wrote:
> 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.)
I originally added assertions when creating the intrinsics. But I add a check in the verifier in this revision.


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

https://reviews.llvm.org/D125291