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

nikic added a comment. In 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 wh

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

xbolva00 added inline comments. Comment at: llvm/include/llvm/IR/Intrinsics.td:1408 +// Intrinsic to wrap a thread local variable. +def int_threadlocal_address : DefaultAttrsIntrinsic<[llvm_anyptr_ty], [LLVMMatchType<0>], + [IntrNoMem, IntrSpeculatable,

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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG970105351710: Introduce @llvm.threadlocal.address intrinsic to access TLS variable (authored by ChuanqiXu). Repository: rG LLVM Github Monorepo C

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

ChuanqiXu added a comment. In D125291#3687295 , @nikic wrote: > LG. Now that LLVM 15 has branched, I think it is safe to land this, and there > will be enough time before LLVM 16 to convert it to the token variant. Yeah, thanks for reviewing! CHANGES

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

ChuanqiXu updated this revision to Diff 448904. ChuanqiXu marked an inline comment as done. ChuanqiXu added a comment. Address comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D125291/new/ https://reviews.llvm.org/D125291 Files: llvm/docs/LangRef.rst llvm/include/llvm/Analysi

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

nikic accepted this revision. nikic added a comment. This revision is now accepted and ready to land. LG. Now that LLVM 15 has branched, I think it is safe to land this, and there will be enough time before LLVM 16 to convert it to the token variant. Comment at: llvm/docs/Lang

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

ChuanqiXu marked an inline comment as done. ChuanqiXu added a comment. In D125291#3651640 , @nhaehnle wrote: > I can't judge the Clang changes. Now the clang part is moved to D129833 . > On the LLVM side, can you also a

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

ChuanqiXu updated this revision to Diff 444880. ChuanqiXu marked 3 inline comments as done. ChuanqiXu added a comment. Address comments and remove verifier due to the automatic merging for constant expression I mentioned above. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D125291/new/

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

ychen added inline comments. Comment at: llvm/docs/LangRef.rst:24541 + + declare ptr @llvm.threadlocal.address(ptr) nounwind readnone willreturn + Like @jyknight already did, any idea what it would take to change the parameter type from pointer to token? Lo

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

nhaehnle added a comment. I can't judge the Clang changes. On the LLVM side, can you also add the implementation and a test for the GlobalISel path? Plus, I have some minor inline comments. Comment at: llvm/docs/LangRef.rst:24546 + +The first argument is pointer, which refers

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

ChuanqiXu added a reviewer: ychen. ChuanqiXu added a comment. @jyknight @nikic @rjmccall @efriedma @nhaehnle gentle ping~ CHANGES SINCE LAST ACTION https://reviews.llvm.org/D125291/new/ https://reviews.llvm.org/D125291 ___ cfe-commits mailing list

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

ChuanqiXu added a reviewer: nikic. ChuanqiXu marked 2 inline comments as done. ChuanqiXu added a comment. In D125291#3629276 , @nikic wrote: > FWIW the bitcode patch has landed, so implementing the variant using a token > type should be possible now. I'm

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

nikic added a comment. 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.

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

ChuanqiXu added a comment. Ping @jyknight Or anyone else will review this one? I want us to fix the thread problems in clang15, which would be created on July 26th. @rjmccall @nhaehnle @danilaml @efriedma CHANGES SINCE LAST ACTION https://reviews.llvm.org/D125291/new/ https://reviews.llvm.

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

ChuanqiXu added a comment. @jyknight ping! Or someone else is willing to review this one? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D125291/new/ https://reviews.llvm.org/D125291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http

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

ChuanqiXu added a comment. @jyknight How do you think about the status now? I want to fix the thread local problem for coroutines in clang15 since the problem have been found for years... CHANGES SINCE LAST ACTION https://reviews.llvm.org/D125291/new/ https://reviews.llvm.org/D125291

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

ChuanqiXu added a comment. In D125291#3581064 , @nikic wrote: > In D125291#3548671 , @jyknight > wrote: > >> Anyhow -- I think the prototype I'm fiddling with is also along the path to >> the ideal long-term sta

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

nikic added a comment. In D125291#3548671 , @jyknight wrote: > 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

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

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 pe

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

ChuanqiXu updated this revision to Diff 435475. ChuanqiXu added a comment. Address inline comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D125291/new/ https://reviews.llvm.org/D125291 Files: clang/lib/CodeGen/CGExpr.cpp clang/lib/CodeGen/ItaniumCXXABI.cpp clang/test/CodeGe

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

ChuanqiXu updated this revision to Diff 435461. ChuanqiXu added a comment. Handle function local thread locals. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D125291/new/ https://reviews.llvm.org/D125291 Files: clang/lib/CodeGen/CGExpr.cpp clang/lib/CodeGen/ItaniumCXXABI.cpp clang

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

ChuanqiXu marked an inline comment as done. ChuanqiXu added a comment. In 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 serie

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

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

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

nhaehnle added inline comments. Comment at: llvm/include/llvm/IR/IRBuilder.h:746-747 + /// Create a call to llvm.threadlocal.address intrinsic. + CallInst *CreateThreadLocalAddress(Value *Ptr); + ChuanqiXu wrote: > nhaehnle wrote: > > This could be a `GlobalV

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

wenlei added a subscriber: weiwang. wenlei added a comment. +@weiwang 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/cg

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

ChuanqiXu marked 2 inline comments as done. ChuanqiXu added inline comments. Comment at: llvm/include/llvm/IR/IRBuilder.h:746-747 + /// Create a call to llvm.threadlocal.address intrinsic. + CallInst *CreateThreadLocalAddress(Value *Ptr); + nhaehnle wrote: >

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

ChuanqiXu updated this revision to Diff 431890. ChuanqiXu added a comment. Address comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D125291/new/ https://reviews.llvm.org/D125291 Files: clang/lib/CodeGen/CGExpr.cpp clang/lib/CodeGen/ItaniumCXXABI.cpp clang/test/CodeGenCXX/cx

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

nhaehnle added inline comments. Comment at: llvm/include/llvm/IR/Intrinsics.td:1393-1394 +def int_threadlocal_address : Intrinsic<[llvm_ptr_ty], [llvm_ptr_ty], +[IntrNoMem, IntrWillReturn]>; + Whether IntrNoMem is truly c

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

nhaehnle added a comment. 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 can't really speak for the Clang parts, but the LLVM parts looks reasonable to me modulo some detail comments. =

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

ChuanqiXu added inline comments. Comment at: llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp:1 //===- PreISelIntrinsicLowering.cpp - Pre-ISel intrinsic lowering pass ===// // I feel this is a better place than SelectDAG. CHANGES SINCE LAST ACTION https:/

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

ChuanqiXu updated this revision to Diff 430585. ChuanqiXu added a comment. Cleanup codes. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D125291/new/ https://reviews.llvm.org/D125291 Files: clang/lib/CodeGen/CGExpr.cpp clang/lib/CodeGen/ItaniumCXXABI.cpp clang/test/CodeGenCXX/cxx11

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

ChuanqiXu updated this revision to Diff 430583. ChuanqiXu added a comment. Address comments: - Lowering the introduced intrinsic in CodeGen pipelines. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D125291/new/ https://reviews.llvm.org/D125291 Files: clang/lib/CodeGen/CGExpr.cpp cla

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

ChuanqiXu added a comment. In D125291#3523818 , @efriedma wrote: > I don't really understand how this is supposed to interact with D125292 > ; even if you strip the readnone attribute > from the call instruction, we'll

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

efriedma added a comment. I don't really understand how this is supposed to interact with D125292 ; even if you strip the readnone attribute from the call instruction, we'll still treat a call to the intrinsic as readnone. I think I'd prefer to lower the intri

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

ChuanqiXu added a comment. @rjmccall @eli.friedman @jyknight gentle ping~ Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D125291/new/ https://reviews.llvm.org/D125291 ___ cfe-commits mailing list cfe-commi