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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
