jyknight added a comment. 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.)
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. 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.) ================ 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. + // ---------------- 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). ================ Comment at: llvm/include/llvm/IR/Intrinsics.td:1393 +def int_threadlocal_address : Intrinsic<[llvm_ptr_ty], [llvm_ptr_ty], + [IntrNoMem, IntrWillReturn]>; ---------------- I believe this should be declared exactly like int_ptrmask right above. 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
