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

[Nikita Popov via Phabricator via cfe-commits]

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

Do you plan to pursue this? We now have the necessary infrastructure to make 
the bitcode upgrade simple, and I think it would be great to include this 
change in LLVM 16, so we directly introduce llvm.threadlocal.address in its 
final form.


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

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

[Dávid Bolvanský via Phabricator via cfe-commits]

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, IntrWillReturn]>;

Returns nonnull?


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

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

[Chuanqi Xu via Phabricator via cfe-commits]

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

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

https://reviews.llvm.org/D125291

Files:
  llvm/docs/LangRef.rst
  llvm/include/llvm/Analysis/TargetTransformInfoImpl.h
  llvm/include/llvm/IR/IRBuilder.h
  llvm/include/llvm/IR/Intrinsics.td
  llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
  llvm/lib/IR/IRBuilder.cpp
  llvm/test/CodeGen/X86/threadlocal_address.ll

Index: llvm/test/CodeGen/X86/threadlocal_address.ll
===
--- /dev/null
+++ llvm/test/CodeGen/X86/threadlocal_address.ll
@@ -0,0 +1,41 @@
+; RUN: llc -mtriple=x86_64-unknown-linux-gnu -stop-after=finalize-isel %s -o - | FileCheck %s
+
+@i = thread_local global i32 0, align 4
+
+define noundef i32 @foo() {
+; CHECK: %0:gr64 = MOV64rm $rip, 1, $noreg, target-flags(x86-gottpoff) @i, $noreg :: (load (s64) from got)
+; CHECK: %1:gr32 = MOV32rm %0, 1, $noreg, 0, $fs :: (load (s32) from %ir.0)
+; CHECK: %2:gr32 = nsw INC32r %1, implicit-def dead $eflags
+; CHECK: MOV32mr %0, 1, $noreg, 0, $fs, %2 :: (store (s32) into %ir.0)
+; CHECK: $eax = COPY %2
+; CHECK: RET 0, $eax
+entry:
+  %0 = call ptr @llvm.threadlocal.address(ptr @i)
+  %1 = load i32, ptr %0, align 4
+  %inc = add nsw i32 %1, 1
+  store i32 %inc, ptr %0, align 4
+  %2 = call ptr @llvm.threadlocal.address(ptr @i)
+  %3 = load i32, ptr %2, align 4
+  ret i32 %3
+}
+
+@j =  thread_local addrspace(1) global  i32 addrspace(0)* @i, align 4
+define noundef i32 @bar() {
+; CHECK: %0:gr64 = MOV64rm $rip, 1, $noreg, target-flags(x86-gottpoff) @j, $noreg :: (load (s64) from got)
+; CHECK: %1:gr32 = MOV32rm %0, 1, $noreg, 0, $fs :: (load (s32) from %ir.0, addrspace 1)
+; CHECK: %2:gr32 = nsw INC32r %1, implicit-def dead $eflags
+; CHECK: MOV32mr %0, 1, $noreg, 0, $fs, %2 :: (store (s32) into %ir.0, addrspace 1)
+; CHECK: $eax = COPY %2
+; CHECK: RET 0, $eax
+entry:
+  %0 = call ptr addrspace(1) @llvm.threadlocal.address.p1(ptr addrspace(1) @j)
+  %1 = load i32, ptr addrspace(1) %0, align 4
+  %inc = add nsw i32 %1, 1
+  store i32 %inc, ptr addrspace(1) %0, align 4
+  %2 = call ptr addrspace(1) @llvm.threadlocal.address.p1(ptr addrspace(1) @j)
+  %3 = load i32, ptr addrspace(1) %2, align 4
+  ret i32 %3
+}
+
+declare ptr @llvm.threadlocal.address(ptr) nounwind readnone willreturn
+declare ptr addrspace(1) @llvm.threadlocal.address.p1(ptr addrspace(1)) nounwind readnone willreturn
Index: llvm/lib/IR/IRBuilder.cpp
===
--- llvm/lib/IR/IRBuilder.cpp
+++ llvm/lib/IR/IRBuilder.cpp
@@ -526,6 +526,13 @@
   return CreateCall(TheFn, Ops);
 }
 
+CallInst *IRBuilderBase::CreateThreadLocalAddress(Value *Ptr) {
+  assert(isa(Ptr) && cast(Ptr)->isThreadLocal() &&
+ "threadlocal_address only applies to thread local variables.");
+  return CreateIntrinsic(llvm::Intrinsic::threadlocal_address, {Ptr->getType()},
+ {Ptr});
+}
+
 CallInst *
 IRBuilderBase::CreateAssumption(Value *Cond,
 ArrayRef OpBundles) {
Index: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
===
--- llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
+++ llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
@@ -7178,6 +7178,10 @@
  DAG.getZExtOrTrunc(Const, sdl, PtrVT)));
 return;
   }
+  case Intrinsic::threadlocal_address: {
+setValue(, getValue(I.getOperand(0)));
+return;
+  }
   case Intrinsic::get_active_lane_mask: {
 EVT CCVT = TLI.getValueType(DAG.getDataLayout(), I.getType());
 SDValue Index = getValue(I.getOperand(0));
Index: llvm/include/llvm/IR/Intrinsics.td
===
--- llvm/include/llvm/IR/Intrinsics.td
+++ llvm/include/llvm/IR/Intrinsics.td
@@ -1404,6 +1404,10 @@
 def int_ptrmask: DefaultAttrsIntrinsic<[llvm_anyptr_ty], [LLVMMatchType<0>, llvm_anyint_ty],
[IntrNoMem, IntrSpeculatable, IntrWillReturn]>;
 
+// Intrinsic to wrap a thread local variable.
+def int_threadlocal_address : DefaultAttrsIntrinsic<[llvm_anyptr_ty], [LLVMMatchType<0>],
+   [IntrNoMem, IntrSpeculatable, IntrWillReturn]>;
+
 def int_experimental_stepvector : DefaultAttrsIntrinsic<[llvm_anyvector_ty],
 [], [IntrNoMem]>;
 
Index: llvm/include/llvm/IR/IRBuilder.h
===
--- llvm/include/llvm/IR/IRBuilder.h
+++ llvm/include/llvm/IR/IRBuilder.h
@@ -753,6 +753,9 @@
   /// If the pointer isn't i8* it will be converted.
   CallInst 

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

[Chuanqi Xu via Phabricator via cfe-commits]

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

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

[Chuanqi Xu via Phabricator via cfe-commits]

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/Analysis/TargetTransformInfoImpl.h
  llvm/include/llvm/IR/IRBuilder.h
  llvm/include/llvm/IR/Intrinsics.td
  llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
  llvm/lib/IR/IRBuilder.cpp
  llvm/test/CodeGen/X86/threadlocal_address.ll

Index: llvm/test/CodeGen/X86/threadlocal_address.ll
===
--- /dev/null
+++ llvm/test/CodeGen/X86/threadlocal_address.ll
@@ -0,0 +1,41 @@
+; RUN: llc -mtriple=x86_64-unknown-linux-gnu -stop-after=finalize-isel %s -o - | FileCheck %s
+
+@i = thread_local global i32 0, align 4
+
+define noundef i32 @foo() {
+; CHECK: %0:gr64 = MOV64rm $rip, 1, $noreg, target-flags(x86-gottpoff) @i, $noreg :: (load (s64) from got)
+; CHECK: %1:gr32 = MOV32rm %0, 1, $noreg, 0, $fs :: (load (s32) from %ir.0)
+; CHECK: %2:gr32 = nsw INC32r %1, implicit-def dead $eflags
+; CHECK: MOV32mr %0, 1, $noreg, 0, $fs, %2 :: (store (s32) into %ir.0)
+; CHECK: $eax = COPY %2
+; CHECK: RET 0, $eax
+entry:
+  %0 = call ptr @llvm.threadlocal.address(ptr @i)
+  %1 = load i32, ptr %0, align 4
+  %inc = add nsw i32 %1, 1
+  store i32 %inc, ptr %0, align 4
+  %2 = call ptr @llvm.threadlocal.address(ptr @i)
+  %3 = load i32, ptr %2, align 4
+  ret i32 %3
+}
+
+@j =  thread_local addrspace(1) global  i32 addrspace(0)* @i, align 4
+define noundef i32 @bar() {
+; CHECK: %0:gr64 = MOV64rm $rip, 1, $noreg, target-flags(x86-gottpoff) @j, $noreg :: (load (s64) from got)
+; CHECK: %1:gr32 = MOV32rm %0, 1, $noreg, 0, $fs :: (load (s32) from %ir.0, addrspace 1)
+; CHECK: %2:gr32 = nsw INC32r %1, implicit-def dead $eflags
+; CHECK: MOV32mr %0, 1, $noreg, 0, $fs, %2 :: (store (s32) into %ir.0, addrspace 1)
+; CHECK: $eax = COPY %2
+; CHECK: RET 0, $eax
+entry:
+  %0 = call ptr addrspace(1) @llvm.threadlocal.address.p1(ptr addrspace(1) @j)
+  %1 = load i32, ptr addrspace(1) %0, align 4
+  %inc = add nsw i32 %1, 1
+  store i32 %inc, ptr addrspace(1) %0, align 4
+  %2 = call ptr addrspace(1) @llvm.threadlocal.address.p1(ptr addrspace(1) @j)
+  %3 = load i32, ptr addrspace(1) %2, align 4
+  ret i32 %3
+}
+
+declare ptr @llvm.threadlocal.address(ptr) nounwind readnone willreturn
+declare ptr addrspace(1) @llvm.threadlocal.address.p1(ptr addrspace(1)) nounwind readnone willreturn
Index: llvm/lib/IR/IRBuilder.cpp
===
--- llvm/lib/IR/IRBuilder.cpp
+++ llvm/lib/IR/IRBuilder.cpp
@@ -526,6 +526,13 @@
   return CreateCall(TheFn, Ops);
 }
 
+CallInst *IRBuilderBase::CreateThreadLocalAddress(Value *Ptr) {
+  assert(isa(Ptr) && cast(Ptr)->isThreadLocal() &&
+ "threadlocal_address only applies to thread local variables.");
+  return CreateIntrinsic(llvm::Intrinsic::threadlocal_address, {Ptr->getType()},
+ {Ptr});
+}
+
 CallInst *
 IRBuilderBase::CreateAssumption(Value *Cond,
 ArrayRef OpBundles) {
Index: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
===
--- llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
+++ llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
@@ -7178,6 +7178,10 @@
  DAG.getZExtOrTrunc(Const, sdl, PtrVT)));
 return;
   }
+  case Intrinsic::threadlocal_address: {
+setValue(, getValue(I.getOperand(0)));
+return;
+  }
   case Intrinsic::get_active_lane_mask: {
 EVT CCVT = TLI.getValueType(DAG.getDataLayout(), I.getType());
 SDValue Index = getValue(I.getOperand(0));
Index: llvm/include/llvm/IR/Intrinsics.td
===
--- llvm/include/llvm/IR/Intrinsics.td
+++ llvm/include/llvm/IR/Intrinsics.td
@@ -1404,6 +1404,10 @@
 def int_ptrmask: DefaultAttrsIntrinsic<[llvm_anyptr_ty], [LLVMMatchType<0>, llvm_anyint_ty],
[IntrNoMem, IntrSpeculatable, IntrWillReturn]>;
 
+// Intrinsic to wrap a thread local variable.
+def int_threadlocal_address : DefaultAttrsIntrinsic<[llvm_anyptr_ty], [LLVMMatchType<0>],
+   [IntrNoMem, IntrSpeculatable, IntrWillReturn]>;
+
 def int_experimental_stepvector : DefaultAttrsIntrinsic<[llvm_anyvector_ty],
 [], [IntrNoMem]>;
 
Index: llvm/include/llvm/IR/IRBuilder.h
===
--- llvm/include/llvm/IR/IRBuilder.h
+++ llvm/include/llvm/IR/IRBuilder.h
@@ -753,6 +753,9 @@
   /// If the pointer isn't i8* it will be converted.
   CallInst *CreateInvariantStart(Value *Ptr, ConstantInt *Size = nullptr);
 
+  /// Create a call to llvm.threadlocal.address intrinsic.
+  CallInst 

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

[Nikita Popov via Phabricator via cfe-commits]

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/LangRef.rst:24546
+
+The first argument is a pointer, which refers to a thread local variable.
+

I'd replace variable -> global here.


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

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

[Chuanqi Xu via Phabricator via cfe-commits]

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 add the implementation and a test for the 
> GlobalISel path? Plus, I have some minor inline comments.

I don't understand it a lot since I lack experience in the backend. I've 
implemented it in SelectionDAG. And it looks redundant to implement it again in 
GlobalISel. May you explain it?




Comment at: llvm/docs/LangRef.rst:24541
+
+  declare ptr @llvm.threadlocal.address(ptr) nounwind readnone willreturn
+

ychen wrote:
> Like @jyknight already did, any idea what it would take to change the 
> parameter type from pointer to token? Looking at the test changes, it looks 
> like OpenMP is impacted a lot. I suggest splitting this patch into Clang and 
> LLVM parts to gain insights from the frontend experts.
The change for OpenMP is relatively large since they used 
utils/update_cc_test_checks.py to update their tests automatically.

I've split the clang part into https://reviews.llvm.org/D129833


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

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

[Chuanqi Xu via Phabricator via cfe-commits]

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/

https://reviews.llvm.org/D125291

Files:
  llvm/docs/LangRef.rst
  llvm/include/llvm/Analysis/TargetTransformInfoImpl.h
  llvm/include/llvm/IR/IRBuilder.h
  llvm/include/llvm/IR/Intrinsics.td
  llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
  llvm/lib/IR/IRBuilder.cpp
  llvm/test/CodeGen/X86/threadlocal_address.ll

Index: llvm/test/CodeGen/X86/threadlocal_address.ll
===
--- /dev/null
+++ llvm/test/CodeGen/X86/threadlocal_address.ll
@@ -0,0 +1,41 @@
+; RUN: llc -mtriple=x86_64-unknown-linux-gnu -stop-after=finalize-isel %s -o - | FileCheck %s
+
+@i = thread_local global i32 0, align 4
+
+define noundef i32 @foo() {
+; CHECK: %0:gr64 = MOV64rm $rip, 1, $noreg, target-flags(x86-gottpoff) @i, $noreg :: (load (s64) from got)
+; CHECK: %1:gr32 = MOV32rm %0, 1, $noreg, 0, $fs :: (load (s32) from %ir.0)
+; CHECK: %2:gr32 = nsw INC32r %1, implicit-def dead $eflags
+; CHECK: MOV32mr %0, 1, $noreg, 0, $fs, %2 :: (store (s32) into %ir.0)
+; CHECK: $eax = COPY %2
+; CHECK: RET 0, $eax
+entry:
+  %0 = call ptr @llvm.threadlocal.address(ptr @i)
+  %1 = load i32, ptr %0, align 4
+  %inc = add nsw i32 %1, 1
+  store i32 %inc, ptr %0, align 4
+  %2 = call ptr @llvm.threadlocal.address(ptr @i)
+  %3 = load i32, ptr %2, align 4
+  ret i32 %3
+}
+
+@j =  thread_local addrspace(1) global  i32 addrspace(0)* @i, align 4
+define noundef i32 @bar() {
+; CHECK: %0:gr64 = MOV64rm $rip, 1, $noreg, target-flags(x86-gottpoff) @j, $noreg :: (load (s64) from got)
+; CHECK: %1:gr32 = MOV32rm %0, 1, $noreg, 0, $fs :: (load (s32) from %ir.0, addrspace 1)
+; CHECK: %2:gr32 = nsw INC32r %1, implicit-def dead $eflags
+; CHECK: MOV32mr %0, 1, $noreg, 0, $fs, %2 :: (store (s32) into %ir.0, addrspace 1)
+; CHECK: $eax = COPY %2
+; CHECK: RET 0, $eax
+entry:
+  %0 = call ptr addrspace(1) @llvm.threadlocal.address.p1(ptr addrspace(1) @j)
+  %1 = load i32, ptr addrspace(1) %0, align 4
+  %inc = add nsw i32 %1, 1
+  store i32 %inc, ptr addrspace(1) %0, align 4
+  %2 = call ptr addrspace(1) @llvm.threadlocal.address.p1(ptr addrspace(1) @j)
+  %3 = load i32, ptr addrspace(1) %2, align 4
+  ret i32 %3
+}
+
+declare ptr @llvm.threadlocal.address(ptr) nounwind readnone willreturn
+declare ptr addrspace(1) @llvm.threadlocal.address.p1(ptr addrspace(1)) nounwind readnone willreturn
Index: llvm/lib/IR/IRBuilder.cpp
===
--- llvm/lib/IR/IRBuilder.cpp
+++ llvm/lib/IR/IRBuilder.cpp
@@ -528,6 +528,13 @@
   return createCallHelper(TheFn, Ops, this);
 }
 
+CallInst *IRBuilderBase::CreateThreadLocalAddress(Value *Ptr) {
+  assert(isa(Ptr) && cast(Ptr)->isThreadLocal() &&
+ "threadlocal_address only applies to thread local variables.");
+  return  CreateIntrinsic(llvm::Intrinsic::threadlocal_address, {Ptr->getType()},
+ {Ptr});
+}
+
 CallInst *
 IRBuilderBase::CreateAssumption(Value *Cond,
 ArrayRef OpBundles) {
Index: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
===
--- llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
+++ llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
@@ -7167,6 +7167,10 @@
  DAG.getZExtOrTrunc(Const, sdl, PtrVT)));
 return;
   }
+  case Intrinsic::threadlocal_address: {
+setValue(, getValue(I.getOperand(0)));
+return;
+  }
   case Intrinsic::get_active_lane_mask: {
 EVT CCVT = TLI.getValueType(DAG.getDataLayout(), I.getType());
 SDValue Index = getValue(I.getOperand(0));
Index: llvm/include/llvm/IR/Intrinsics.td
===
--- llvm/include/llvm/IR/Intrinsics.td
+++ llvm/include/llvm/IR/Intrinsics.td
@@ -1402,6 +1402,10 @@
 def int_ptrmask: DefaultAttrsIntrinsic<[llvm_anyptr_ty], [LLVMMatchType<0>, llvm_anyint_ty],
[IntrNoMem, IntrSpeculatable, IntrWillReturn]>;
 
+// Intrinsic to wrap a thread local variable.
+def int_threadlocal_address : DefaultAttrsIntrinsic<[llvm_anyptr_ty], [LLVMMatchType<0>],
+   [IntrNoMem, IntrSpeculatable, IntrWillReturn]>;
+
 def int_experimental_stepvector : DefaultAttrsIntrinsic<[llvm_anyvector_ty],
 [], [IntrNoMem]>;
 
Index: llvm/include/llvm/IR/IRBuilder.h
===
--- llvm/include/llvm/IR/IRBuilder.h
+++ llvm/include/llvm/IR/IRBuilder.h
@@ -749,6 +749,9 @@
   /// If the pointer isn't i8* it will be converted.
   CallInst *CreateInvariantStart(Value *Ptr, ConstantInt *Size = 

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

[Yuanfang Chen via Phabricator via cfe-commits]

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? Looking at the test changes, it looks like OpenMP 
is impacted a lot. I suggest splitting this patch into Clang and LLVM parts to 
gain insights from the frontend experts.



Comment at: llvm/lib/IR/IRBuilder.cpp:533
+  if (!isa(Ptr) || !cast(Ptr)->isThreadLocal()) {
+llvm::outs() << "Non Global Value wanted.\n";
+Ptr->dump();

Here and below `llvm::outs()` are for debugging and should be removed?


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

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

[Nicolai Hähnle via Phabricator via cfe-commits]

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 to a thread local variable.
+

The first argument is **a** pointer



Comment at: llvm/docs/LangRef.rst:24551-24553
+The LLVM treated the address of thread local variable as a constant expression.
+But it is not true. The ``llvm.threadlocal.address`` intrinsic would represent
+the address of the thread local variable.

LangRef should focus on how IR **is** defined, not the historical evolution. I 
think everything that needs to be said here is actually already said in the 
Semantics section.


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

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

[Chuanqi Xu via Phabricator via cfe-commits]

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

[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 , @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 = 

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

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

[Nikita Popov via Phabricator via cfe-commits]

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.




Comment at: llvm/docs/LangRef.rst:24415
+
+  declare ptr @llvm.threadlocal.address(ptr) nounwind readnone willreturn
+

Don't we need to overload this intrinsic by return type, so it works with 
different address spaces?



Comment at: llvm/docs/LangRef.rst:24420
+
+The first argument is pointer, which refers to a thread local variable.
+

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



Comment at: llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp:231
+  Changed |= lowerThreadLocalIntrinsics(F);
+  break;
 }

I don't think this belongs here, this should get dropped by SelectionDAGBuilder.



Comment at: 
llvm/test/Transforms/PreISelIntrinsicLowering/threadlocal_address.ll:1
+; RUN: opt -pre-isel-intrinsic-lowering -opaque-pointers -S -o - < %s | 
FileCheck %s
+

`-opaque-pointers` flag is not necessary.


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

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

[Chuanqi Xu via Phabricator via cfe-commits]

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.org/D125291

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Chuanqi Xu via Phabricator via cfe-commits]

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

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

[Chuanqi Xu via Phabricator via cfe-commits]

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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Chuanqi Xu via Phabricator via cfe-commits]

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 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.)
>
> I have implemented support for converting constant expressions to 
> instructions in the bitcode reader in https://reviews.llvm.org/D127729. This 
> was originally intended for constant expression removal, but I think with 
> that infrastructure in place, upgrading TLS global references to use an 
> intrinsic would be fairly simple.

Oh, it looks great! Especially in `getValueForInitializer`, we could solve the 
motivation problem given by @jyknight :

  @x = thread_local global i32 0, align 4
  @y = global i32* @x, align 8

Now we are possible to emit proper diagnostic message. The only issue is that 
the current framework of D127729  would map a 
constexpr into an instruction. But we might need to map a constexpr with TLS 
variables to multiple instructions (containing @llvm.threadaddress() 
intrinsics). But I believe this problem might be minor.

@jyknight How do you think about the status quo now?


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

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

[Nikita Popov via Phabricator via cfe-commits]

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

I have implemented support for converting constant expressions to instructions 
in the bitcode reader in https://reviews.llvm.org/D127729. This was originally 
intended for constant expression removal, but I think with that infrastructure 
in place, upgrading TLS global references to use an intrinsic would be fairly 
simple.


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

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

[Chuanqi Xu via Phabricator via cfe-commits]

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 perspective, it is fine since 
the initializers should never be coroutines. (I meant to fix the coroutines 
bugs at the very start).
(2) The IR upgrader problem. It is fine to me too since we won't block the use 
of TLS variable directly after the patch landed (maybe we would in the longer 
future). So the higher version of LLVM will be able to compile the IR from 
older version after the patch landed. So it is not a problem to me. (It looks 
like the backward compatibility is not emphasized. This is the first time I saw 
the problem in the community)




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

jyknight wrote:
> 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).
Yeah, it could be handled by an overloaded intrinsic. But given the process of 
opaque pointers goes well really, I feel like it is redundant to support non 
opaque pointer mode. It shouldn't affect users since opaque pointers is enabled 
by default as far as I know.


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

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

[Chuanqi Xu via Phabricator via cfe-commits]

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/CodeGenCXX/cxx11-thread-local-instantiated.cpp
  clang/test/CodeGenCXX/pr18635.cpp
  clang/test/CodeGenCXX/threadlocal_address.cpp
  llvm/docs/LangRef.rst
  llvm/include/llvm/Analysis/TargetTransformInfoImpl.h
  llvm/include/llvm/IR/IRBuilder.h
  llvm/include/llvm/IR/Intrinsics.td
  llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
  llvm/lib/IR/IRBuilder.cpp
  llvm/test/Transforms/PreISelIntrinsicLowering/threadlocal_address.ll

Index: llvm/test/Transforms/PreISelIntrinsicLowering/threadlocal_address.ll
===
--- /dev/null
+++ llvm/test/Transforms/PreISelIntrinsicLowering/threadlocal_address.ll
@@ -0,0 +1,25 @@
+; RUN: opt -pre-isel-intrinsic-lowering -opaque-pointers -S -o - < %s | FileCheck %s
+
+@i = thread_local global i32 0, align 4
+
+define dso_local noundef i32 @foo() {
+; CHECK-LABEL: @foo(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:[[TMP0:%.*]] = load i32, ptr @i, align 4
+; CHECK-NEXT:[[INC:%.*]] = add nsw i32 [[TMP0]], 1
+; CHECK-NEXT:store i32 [[INC]], ptr @i, align 4
+; CHECK-NEXT:[[TMP1:%.*]] = load i32, ptr @i, align 4
+; CHECK-NEXT:ret i32 [[TMP1]]
+;
+; CHECK-NOT: call{{.*}}@llvm.threadlocal.address(
+entry:
+  %0 = call ptr @llvm.threadlocal.address(ptr @i)
+  %1 = load i32, ptr %0, align 4
+  %inc = add nsw i32 %1, 1
+  store i32 %inc, ptr %0, align 4
+  %2 = call ptr @llvm.threadlocal.address(ptr @i)
+  %3 = load i32, ptr %2, align 4
+  ret i32 %3
+}
+
+declare ptr @llvm.threadlocal.address(ptr) nounwind readnone willreturn
Index: llvm/lib/IR/IRBuilder.cpp
===
--- llvm/lib/IR/IRBuilder.cpp
+++ llvm/lib/IR/IRBuilder.cpp
@@ -499,6 +499,13 @@
   return createCallHelper(TheFn, Ops, this);
 }
 
+CallInst *IRBuilderBase::CreateThreadLocalAddress(Value *Ptr) {
+  assert(isa(Ptr) && cast(Ptr)->isThreadLocal() &&
+ "threadlocal_address only applies to thread local variables.");
+  return CreateIntrinsic(llvm::Intrinsic::threadlocal_address, llvm::None,
+ {Ptr});
+}
+
 CallInst *
 IRBuilderBase::CreateAssumption(Value *Cond,
 ArrayRef OpBundles) {
Index: llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
===
--- llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
+++ llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
@@ -6,8 +6,8 @@
 //
 //===--===//
 //
-// This pass implements IR lowering for the llvm.load.relative and llvm.objc.*
-// intrinsics.
+// This pass implements IR lowering for the llvm.threadlocal_address,
+// llvm.load.relative and llvm.objc.* intrinsics.
 //
 //===--===//
 
@@ -128,6 +128,19 @@
   return true;
 }
 
+static bool lowerThreadLocalIntrinsics(Function ) {
+  if (F.use_empty())
+return false;
+
+  for (Use  : llvm::make_early_inc_range(F.uses())) {
+auto *CB = cast(U.getUser());
+CB->replaceAllUsesWith(CB->getOperand(0));
+CB->eraseFromParent();
+  }
+
+  return true;
+}
+
 static bool lowerIntrinsics(Module ) {
   bool Changed = false;
   for (Function  : M) {
@@ -213,6 +226,9 @@
 case Intrinsic::objc_sync_exit:
   Changed |= lowerObjCCall(F, "objc_sync_exit");
   break;
+case Intrinsic::threadlocal_address:
+  Changed |= lowerThreadLocalIntrinsics(F);
+  break;
 }
   }
   return Changed;
Index: llvm/include/llvm/IR/Intrinsics.td
===
--- llvm/include/llvm/IR/Intrinsics.td
+++ llvm/include/llvm/IR/Intrinsics.td
@@ -1390,6 +1390,10 @@
 def int_ptrmask: DefaultAttrsIntrinsic<[llvm_anyptr_ty], [LLVMMatchType<0>, llvm_anyint_ty],
[IntrNoMem, IntrSpeculatable, IntrWillReturn]>;
 
+// Intrinsic to wrap a thread local variable.
+def int_threadlocal_address : DefaultAttrsIntrinsic<[llvm_ptr_ty], [llvm_ptr_ty],
+   [IntrNoMem, IntrSpeculatable, IntrWillReturn]>;
+
 def int_experimental_stepvector : DefaultAttrsIntrinsic<[llvm_anyvector_ty],
 [], [IntrNoMem]>;
 
Index: llvm/include/llvm/IR/IRBuilder.h
===
--- llvm/include/llvm/IR/IRBuilder.h
+++ llvm/include/llvm/IR/IRBuilder.h
@@ -743,6 +743,9 @@
   /// If the pointer isn't i8* it will be converted.
   CallInst *CreateInvariantStart(Value *Ptr, ConstantInt *Size = nullptr);
 
+  /// Create a call to llvm.threadlocal.address intrinsic.
+  CallInst *CreateThreadLocalAddress(Value *Ptr);
+
   

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

[Chuanqi Xu via Phabricator via cfe-commits]

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/test/CodeGenCXX/cxx11-thread-local-instantiated.cpp
  clang/test/CodeGenCXX/pr18635.cpp
  clang/test/CodeGenCXX/threadlocal_address.cpp
  llvm/docs/LangRef.rst
  llvm/include/llvm/Analysis/TargetTransformInfoImpl.h
  llvm/include/llvm/IR/IRBuilder.h
  llvm/include/llvm/IR/Intrinsics.td
  llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
  llvm/lib/IR/IRBuilder.cpp
  llvm/test/Transforms/PreISelIntrinsicLowering/threadlocal_address.ll

Index: llvm/test/Transforms/PreISelIntrinsicLowering/threadlocal_address.ll
===
--- /dev/null
+++ llvm/test/Transforms/PreISelIntrinsicLowering/threadlocal_address.ll
@@ -0,0 +1,25 @@
+; RUN: opt -pre-isel-intrinsic-lowering -opaque-pointers -S -o - < %s | FileCheck %s
+
+@i = thread_local global i32 0, align 4
+
+define dso_local noundef i32 @foo() {
+; CHECK-LABEL: @foo(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:[[TMP0:%.*]] = load i32, ptr @i, align 4
+; CHECK-NEXT:[[INC:%.*]] = add nsw i32 [[TMP0]], 1
+; CHECK-NEXT:store i32 [[INC]], ptr @i, align 4
+; CHECK-NEXT:[[TMP1:%.*]] = load i32, ptr @i, align 4
+; CHECK-NEXT:ret i32 [[TMP1]]
+;
+; CHECK-NOT: call{{.*}}@llvm.threadlocal.address(
+entry:
+  %0 = call ptr @llvm.threadlocal.address(ptr @i)
+  %1 = load i32, ptr %0, align 4
+  %inc = add nsw i32 %1, 1
+  store i32 %inc, ptr %0, align 4
+  %2 = call ptr @llvm.threadlocal.address(ptr @i)
+  %3 = load i32, ptr %2, align 4
+  ret i32 %3
+}
+
+declare ptr @llvm.threadlocal.address(ptr) nounwind readnone willreturn
Index: llvm/lib/IR/IRBuilder.cpp
===
--- llvm/lib/IR/IRBuilder.cpp
+++ llvm/lib/IR/IRBuilder.cpp
@@ -499,6 +499,13 @@
   return createCallHelper(TheFn, Ops, this);
 }
 
+CallInst *IRBuilderBase::CreateThreadLocalAddress(Value *Ptr) {
+  assert(isa(Ptr) && cast(Ptr)->isThreadLocal() &&
+ "threadlocal_address only applies to thread local variables.");
+  return CreateIntrinsic(llvm::Intrinsic::threadlocal_address, llvm::None,
+ {Ptr});
+}
+
 CallInst *
 IRBuilderBase::CreateAssumption(Value *Cond,
 ArrayRef OpBundles) {
Index: llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
===
--- llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
+++ llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
@@ -6,8 +6,8 @@
 //
 //===--===//
 //
-// This pass implements IR lowering for the llvm.load.relative and llvm.objc.*
-// intrinsics.
+// This pass implements IR lowering for the llvm.threadlocal_address,
+// llvm.load.relative and llvm.objc.* intrinsics.
 //
 //===--===//
 
@@ -128,6 +128,19 @@
   return true;
 }
 
+static bool lowerThreadLocalIntrinsics(Function ) {
+  if (F.use_empty())
+return false;
+
+  for (Use  : llvm::make_early_inc_range(F.uses())) {
+auto *CB = cast(U.getUser());
+CB->replaceAllUsesWith(CB->getOperand(0));
+CB->eraseFromParent();
+  }
+
+  return true;
+}
+
 static bool lowerIntrinsics(Module ) {
   bool Changed = false;
   for (Function  : M) {
@@ -213,6 +226,9 @@
 case Intrinsic::objc_sync_exit:
   Changed |= lowerObjCCall(F, "objc_sync_exit");
   break;
+case Intrinsic::threadlocal_address:
+  Changed |= lowerThreadLocalIntrinsics(F);
+  break;
 }
   }
   return Changed;
Index: llvm/include/llvm/IR/Intrinsics.td
===
--- llvm/include/llvm/IR/Intrinsics.td
+++ llvm/include/llvm/IR/Intrinsics.td
@@ -1390,6 +1390,9 @@
 def int_ptrmask: DefaultAttrsIntrinsic<[llvm_anyptr_ty], [LLVMMatchType<0>, llvm_anyint_ty],
[IntrNoMem, IntrSpeculatable, IntrWillReturn]>;
 
+def int_threadlocal_address : Intrinsic<[llvm_ptr_ty], [llvm_ptr_ty],
+[IntrNoMem, IntrWillReturn]>;
+
 def int_experimental_stepvector : DefaultAttrsIntrinsic<[llvm_anyvector_ty],
 [], [IntrNoMem]>;
 
Index: llvm/include/llvm/IR/IRBuilder.h
===
--- llvm/include/llvm/IR/IRBuilder.h
+++ llvm/include/llvm/IR/IRBuilder.h
@@ -743,6 +743,9 @@
   /// If the pointer isn't i8* it will be converted.
   CallInst *CreateInvariantStart(Value *Ptr, ConstantInt *Size = nullptr);
 
+  /// Create a call to llvm.threadlocal.address intrinsic.
+  CallInst *CreateThreadLocalAddress(Value *Ptr);
+
   /// Create a call to Masked Load intrinsic
   

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

[Chuanqi Xu via Phabricator via cfe-commits]

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 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 , @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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[James Y Knight via Phabricator via cfe-commits]

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

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

[Nicolai Hähnle via Phabricator via cfe-commits]

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 `GlobalValue*` operand to reduce the risk of misuse, right?
> We could use `Value*` here to keep consistency with other functions and the 
> uses. We added assertion in implementations to avoid misuses.
Thanks!



Comment at: llvm/include/llvm/IR/Intrinsics.td:1393-1394
 
+def int_threadlocal_address : Intrinsic<[llvm_ptr_ty], [llvm_ptr_ty],
+[IntrNoMem, IntrWillReturn]>;
+

ChuanqiXu wrote:
> nhaehnle wrote:
> > Whether IntrNoMem is truly correct here depends on the overall solution of 
> > the thread identification issue, i.e. it depends on whether readnone 
> > implies "doesn't read thread ID". We'd best discuss that separately.
> Yeah, let's discuss this in discourse thread.
Just to follow up, based on the latest comments including that of @jyknight, 
IntroNoMem is correct.


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

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

[Wenlei He via Phabricator via cfe-commits]

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/cgi-bin/mailman/listinfo/cfe-commits

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

[Chuanqi Xu via Phabricator via cfe-commits]

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:
> This could be a `GlobalValue*` operand to reduce the risk of misuse, right?
We could use `Value*` here to keep consistency with other functions and the 
uses. We added assertion in implementations to avoid misuses.



Comment at: llvm/include/llvm/IR/Intrinsics.td:1393-1394
 
+def int_threadlocal_address : Intrinsic<[llvm_ptr_ty], [llvm_ptr_ty],
+[IntrNoMem, IntrWillReturn]>;
+

nhaehnle wrote:
> Whether IntrNoMem is truly correct here depends on the overall solution of 
> the thread identification issue, i.e. it depends on whether readnone implies 
> "doesn't read thread ID". We'd best discuss that separately.
Yeah, let's discuss this in discourse thread.


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

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

[Chuanqi Xu via Phabricator via cfe-commits]

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/cxx11-thread-local-instantiated.cpp
  clang/test/CodeGenCXX/pr18635.cpp
  clang/test/CodeGenCXX/threadlocal_address.cpp
  llvm/docs/LangRef.rst
  llvm/include/llvm/Analysis/TargetTransformInfoImpl.h
  llvm/include/llvm/IR/IRBuilder.h
  llvm/include/llvm/IR/Intrinsics.td
  llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
  llvm/lib/IR/IRBuilder.cpp
  llvm/test/Transforms/PreISelIntrinsicLowering/threadlocal_address.ll

Index: llvm/test/Transforms/PreISelIntrinsicLowering/threadlocal_address.ll
===
--- /dev/null
+++ llvm/test/Transforms/PreISelIntrinsicLowering/threadlocal_address.ll
@@ -0,0 +1,25 @@
+; RUN: opt -pre-isel-intrinsic-lowering -opaque-pointers -S -o - < %s | FileCheck %s
+
+@i = thread_local global i32 0, align 4
+
+define dso_local noundef i32 @foo() {
+; CHECK-LABEL: @foo(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:[[TMP0:%.*]] = load i32, ptr @i, align 4
+; CHECK-NEXT:[[INC:%.*]] = add nsw i32 [[TMP0]], 1
+; CHECK-NEXT:store i32 [[INC]], ptr @i, align 4
+; CHECK-NEXT:[[TMP1:%.*]] = load i32, ptr @i, align 4
+; CHECK-NEXT:ret i32 [[TMP1]]
+;
+; CHECK-NOT: call{{.*}}@llvm.threadlocal.address(
+entry:
+  %0 = call ptr @llvm.threadlocal.address(ptr @i)
+  %1 = load i32, ptr %0, align 4
+  %inc = add nsw i32 %1, 1
+  store i32 %inc, ptr %0, align 4
+  %2 = call ptr @llvm.threadlocal.address(ptr @i)
+  %3 = load i32, ptr %2, align 4
+  ret i32 %3
+}
+
+declare ptr @llvm.threadlocal.address(ptr) nounwind readnone willreturn
Index: llvm/lib/IR/IRBuilder.cpp
===
--- llvm/lib/IR/IRBuilder.cpp
+++ llvm/lib/IR/IRBuilder.cpp
@@ -499,6 +499,13 @@
   return createCallHelper(TheFn, Ops, this);
 }
 
+CallInst *IRBuilderBase::CreateThreadLocalAddress(Value *Ptr) {
+  assert(isa(Ptr) && cast(Ptr)->isThreadLocal() &&
+ "threadlocal_address only applies to thread local variables.");
+  return CreateIntrinsic(llvm::Intrinsic::threadlocal_address, llvm::None,
+ {Ptr});
+}
+
 CallInst *
 IRBuilderBase::CreateAssumption(Value *Cond,
 ArrayRef OpBundles) {
Index: llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
===
--- llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
+++ llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
@@ -6,8 +6,8 @@
 //
 //===--===//
 //
-// This pass implements IR lowering for the llvm.load.relative and llvm.objc.*
-// intrinsics.
+// This pass implements IR lowering for the llvm.threadlocal_address, 
+// llvm.load.relative and llvm.objc.* intrinsics.
 //
 //===--===//
 
@@ -128,6 +128,19 @@
   return true;
 }
 
+static bool lowerThreadLocalIntrinsics(Function ) {
+  if (F.use_empty())
+return false;
+
+  for (Use  : llvm::make_early_inc_range(F.uses())) {
+auto *CB = cast(U.getUser());
+CB->replaceAllUsesWith(CB->getOperand(0));
+CB->eraseFromParent();
+  }
+
+  return true;
+}
+
 static bool lowerIntrinsics(Module ) {
   bool Changed = false;
   for (Function  : M) {
@@ -213,6 +226,9 @@
 case Intrinsic::objc_sync_exit:
   Changed |= lowerObjCCall(F, "objc_sync_exit");
   break;
+case Intrinsic::threadlocal_address:
+  Changed |= lowerThreadLocalIntrinsics(F);
+  break;
 }
   }
   return Changed;
Index: llvm/include/llvm/IR/Intrinsics.td
===
--- llvm/include/llvm/IR/Intrinsics.td
+++ llvm/include/llvm/IR/Intrinsics.td
@@ -1390,6 +1390,9 @@
 def int_ptrmask: DefaultAttrsIntrinsic<[llvm_anyptr_ty], [LLVMMatchType<0>, llvm_anyint_ty],
[IntrNoMem, IntrSpeculatable, IntrWillReturn]>;
 
+def int_threadlocal_address : Intrinsic<[llvm_ptr_ty], [llvm_ptr_ty],
+[IntrNoMem, IntrWillReturn]>;
+
 def int_experimental_stepvector : DefaultAttrsIntrinsic<[llvm_anyvector_ty],
 [], [IntrNoMem]>;
 
Index: llvm/include/llvm/IR/IRBuilder.h
===
--- llvm/include/llvm/IR/IRBuilder.h
+++ llvm/include/llvm/IR/IRBuilder.h
@@ -743,6 +743,9 @@
   /// If the pointer isn't i8* it will be converted.
   CallInst *CreateInvariantStart(Value *Ptr, ConstantInt *Size = nullptr);
 
+  /// Create a call to llvm.threadlocal.address intrinsic.
+  CallInst *CreateThreadLocalAddress(Value *Ptr);
+
   /// Create a call to Masked Load intrinsic
   CallInst 

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

[Nicolai Hähnle via Phabricator via cfe-commits]

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 correct here depends on the overall solution of the 
thread identification issue, i.e. it depends on whether readnone implies 
"doesn't read thread ID". We'd best discuss that separately.


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

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

[Nicolai Hähnle via Phabricator via cfe-commits]

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.




Comment at: llvm/docs/LangRef.rst:24392-24403
+Overview:
+""
+
+The LLVM treated the address of thread local variable as a constant expression.
+But it is not true. The ``llvm.threadlocal.address`` intrinsic would represent
+the address of the thread local variable.
+

LangRef should be written in present tense. Something like:

> The address of a thread local variable is not a constant, since it depends on 
> the calling thread. The ``llvm.threadlocal.address`` intrinsic returns the 
> address of the given thread local variable in the calling thread.



Comment at: llvm/include/llvm/IR/IRBuilder.h:746-747
 
+  /// Create a call to llvm.threadlocal.address intrinsic.
+  CallInst *CreateThreadLocalAddress(Value *Ptr);
+

This could be a `GlobalValue*` operand to reduce the risk of misuse, right?


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

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

[Chuanqi Xu via Phabricator via cfe-commits]

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://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

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

[Chuanqi Xu via Phabricator via cfe-commits]

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-thread-local-instantiated.cpp
  clang/test/CodeGenCXX/pr18635.cpp
  clang/test/CodeGenCXX/threadlocal_address.cpp
  llvm/docs/LangRef.rst
  llvm/include/llvm/Analysis/TargetTransformInfoImpl.h
  llvm/include/llvm/IR/IRBuilder.h
  llvm/include/llvm/IR/Intrinsics.td
  llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
  llvm/lib/IR/IRBuilder.cpp
  llvm/test/Transforms/PreISelIntrinsicLowering/threadlocal_address.ll

Index: llvm/test/Transforms/PreISelIntrinsicLowering/threadlocal_address.ll
===
--- /dev/null
+++ llvm/test/Transforms/PreISelIntrinsicLowering/threadlocal_address.ll
@@ -0,0 +1,25 @@
+; RUN: opt -pre-isel-intrinsic-lowering -opaque-pointers -S -o - < %s | FileCheck %s
+
+@i = thread_local global i32 0, align 4
+
+define dso_local noundef i32 @foo() {
+; CHECK-LABEL: @foo(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:[[TMP0:%.*]] = load i32, ptr @i, align 4
+; CHECK-NEXT:[[INC:%.*]] = add nsw i32 [[TMP0]], 1
+; CHECK-NEXT:store i32 [[INC]], ptr @i, align 4
+; CHECK-NEXT:[[TMP1:%.*]] = load i32, ptr @i, align 4
+; CHECK-NEXT:ret i32 [[TMP1]]
+;
+; CHECK-NOT: call{{.*}}@llvm.threadlocal.address(
+entry:
+  %0 = call ptr @llvm.threadlocal.address(ptr @i)
+  %1 = load i32, ptr %0, align 4
+  %inc = add nsw i32 %1, 1
+  store i32 %inc, ptr %0, align 4
+  %2 = call ptr @llvm.threadlocal.address(ptr @i)
+  %3 = load i32, ptr %2, align 4
+  ret i32 %3
+}
+
+declare ptr @llvm.threadlocal.address(ptr) nounwind readnone willreturn
Index: llvm/lib/IR/IRBuilder.cpp
===
--- llvm/lib/IR/IRBuilder.cpp
+++ llvm/lib/IR/IRBuilder.cpp
@@ -499,6 +499,11 @@
   return createCallHelper(TheFn, Ops, this);
 }
 
+CallInst *IRBuilderBase::CreateThreadLocalAddress(Value *Ptr) {
+  return CreateIntrinsic(llvm::Intrinsic::threadlocal_address, llvm::None,
+ {Ptr});
+}
+
 CallInst *
 IRBuilderBase::CreateAssumption(Value *Cond,
 ArrayRef OpBundles) {
Index: llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
===
--- llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
+++ llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
@@ -6,8 +6,8 @@
 //
 //===--===//
 //
-// This pass implements IR lowering for the llvm.load.relative and llvm.objc.*
-// intrinsics.
+// This pass implements IR lowering for the llvm.threadlocal_address, 
+// llvm.load.relative and llvm.objc.* intrinsics.
 //
 //===--===//
 
@@ -128,6 +128,19 @@
   return true;
 }
 
+static bool lowerThreadLocalIntrinsics(Function ) {
+  if (F.use_empty())
+return false;
+
+  for (Use  : llvm::make_early_inc_range(F.uses())) {
+auto *CB = cast(U.getUser());
+CB->replaceAllUsesWith(CB->getOperand(0));
+CB->eraseFromParent();
+  }
+
+  return true;
+}
+
 static bool lowerIntrinsics(Module ) {
   bool Changed = false;
   for (Function  : M) {
@@ -213,6 +226,9 @@
 case Intrinsic::objc_sync_exit:
   Changed |= lowerObjCCall(F, "objc_sync_exit");
   break;
+case Intrinsic::threadlocal_address:
+  Changed |= lowerThreadLocalIntrinsics(F);
+  break;
 }
   }
   return Changed;
Index: llvm/include/llvm/IR/Intrinsics.td
===
--- llvm/include/llvm/IR/Intrinsics.td
+++ llvm/include/llvm/IR/Intrinsics.td
@@ -1390,6 +1390,9 @@
 def int_ptrmask: DefaultAttrsIntrinsic<[llvm_anyptr_ty], [LLVMMatchType<0>, llvm_anyint_ty],
[IntrNoMem, IntrSpeculatable, IntrWillReturn]>;
 
+def int_threadlocal_address : Intrinsic<[llvm_ptr_ty], [llvm_ptr_ty],
+[IntrNoMem, IntrWillReturn]>;
+
 def int_experimental_stepvector : DefaultAttrsIntrinsic<[llvm_anyvector_ty],
 [], [IntrNoMem]>;
 
Index: llvm/include/llvm/IR/IRBuilder.h
===
--- llvm/include/llvm/IR/IRBuilder.h
+++ llvm/include/llvm/IR/IRBuilder.h
@@ -743,6 +743,9 @@
   /// If the pointer isn't i8* it will be converted.
   CallInst *CreateInvariantStart(Value *Ptr, ConstantInt *Size = nullptr);
 
+  /// Create a call to llvm.threadlocal.address intrinsic.
+  CallInst *CreateThreadLocalAddress(Value *Ptr);
+
   /// Create a call to Masked Load intrinsic
   CallInst *CreateMaskedLoad(Type *Ty, Value *Ptr, Align Alignment, Value *Mask,
  Value *PassThru = nullptr, const Twine  = 

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

[Chuanqi Xu via Phabricator via cfe-commits]

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
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  clang/test/CodeGenCXX/cxx11-thread-local-instantiated.cpp
  clang/test/CodeGenCXX/pr18635.cpp
  clang/test/CodeGenCXX/threadlocal_address.cpp
  llvm/docs/LangRef.rst
  llvm/include/llvm/Analysis/TargetTransformInfoImpl.h
  llvm/include/llvm/IR/IRBuilder.h
  llvm/include/llvm/IR/Intrinsics.td
  llvm/lib/CodeGen/IntrinsicLowering.cpp
  llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
  llvm/lib/IR/IRBuilder.cpp
  llvm/test/Transforms/PreISelIntrinsicLowering/threadlocal_address.ll

Index: llvm/test/Transforms/PreISelIntrinsicLowering/threadlocal_address.ll
===
--- /dev/null
+++ llvm/test/Transforms/PreISelIntrinsicLowering/threadlocal_address.ll
@@ -0,0 +1,25 @@
+; RUN: opt -pre-isel-intrinsic-lowering -opaque-pointers -S -o - < %s | FileCheck %s
+
+@i = thread_local global i32 0, align 4
+
+define dso_local noundef i32 @foo() {
+; CHECK-LABEL: @foo(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:[[TMP0:%.*]] = load i32, ptr @i, align 4
+; CHECK-NEXT:[[INC:%.*]] = add nsw i32 [[TMP0]], 1
+; CHECK-NEXT:store i32 [[INC]], ptr @i, align 4
+; CHECK-NEXT:[[TMP1:%.*]] = load i32, ptr @i, align 4
+; CHECK-NEXT:ret i32 [[TMP1]]
+;
+; CHECK-NOT: call{{.*}}@llvm.threadlocal.address(
+entry:
+  %0 = call ptr @llvm.threadlocal.address(ptr @i)
+  %1 = load i32, ptr %0, align 4
+  %inc = add nsw i32 %1, 1
+  store i32 %inc, ptr %0, align 4
+  %2 = call ptr @llvm.threadlocal.address(ptr @i)
+  %3 = load i32, ptr %2, align 4
+  ret i32 %3
+}
+
+declare ptr @llvm.threadlocal.address(ptr) nounwind readnone willreturn
Index: llvm/lib/IR/IRBuilder.cpp
===
--- llvm/lib/IR/IRBuilder.cpp
+++ llvm/lib/IR/IRBuilder.cpp
@@ -499,6 +499,11 @@
   return createCallHelper(TheFn, Ops, this);
 }
 
+CallInst *IRBuilderBase::CreateThreadLocalAddress(Value *Ptr) {
+  return CreateIntrinsic(llvm::Intrinsic::threadlocal_address, llvm::None,
+ {Ptr});
+}
+
 CallInst *
 IRBuilderBase::CreateAssumption(Value *Cond,
 ArrayRef OpBundles) {
Index: llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
===
--- llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
+++ llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
@@ -6,8 +6,8 @@
 //
 //===--===//
 //
-// This pass implements IR lowering for the llvm.load.relative and llvm.objc.*
-// intrinsics.
+// This pass implements IR lowering for the llvm.threadlocal_address, 
+// llvm.load.relative and llvm.objc.* intrinsics.
 //
 //===--===//
 
@@ -128,6 +128,19 @@
   return true;
 }
 
+static bool lowerThreadLocalIntrinsics(Function ) {
+  if (F.use_empty())
+return false;
+
+  for (Use  : llvm::make_early_inc_range(F.uses())) {
+auto *CB = cast(U.getUser());
+CB->replaceAllUsesWith(CB->getOperand(0));
+CB->eraseFromParent();
+  }
+
+  return true;
+}
+
 static bool lowerIntrinsics(Module ) {
   bool Changed = false;
   for (Function  : M) {
@@ -213,6 +226,9 @@
 case Intrinsic::objc_sync_exit:
   Changed |= lowerObjCCall(F, "objc_sync_exit");
   break;
+case Intrinsic::threadlocal_address:
+  Changed |= lowerThreadLocalIntrinsics(F);
+  break;
 }
   }
   return Changed;
Index: llvm/lib/CodeGen/IntrinsicLowering.cpp
===
--- llvm/lib/CodeGen/IntrinsicLowering.cpp
+++ llvm/lib/CodeGen/IntrinsicLowering.cpp
@@ -313,6 +313,10 @@
 break;
   }
 
+  // case Intrinsic::threadlocal_address:
+  //   CI->replaceAllUsesWith(CI->getOperand(0));
+  //   break;
+
   case Intrinsic::dbg_declare:
   case Intrinsic::dbg_label:
 break;// Simply strip out debugging intrinsics
Index: llvm/include/llvm/IR/Intrinsics.td
===
--- llvm/include/llvm/IR/Intrinsics.td
+++ llvm/include/llvm/IR/Intrinsics.td
@@ -1390,6 +1390,9 @@
 def int_ptrmask: DefaultAttrsIntrinsic<[llvm_anyptr_ty], [LLVMMatchType<0>, llvm_anyint_ty],
[IntrNoMem, IntrSpeculatable, IntrWillReturn]>;
 
+def int_threadlocal_address : Intrinsic<[llvm_ptr_ty], [llvm_ptr_ty],
+[IntrNoMem, IntrWillReturn]>;
+
 def int_experimental_stepvector : DefaultAttrsIntrinsic<[llvm_anyvector_ty],
 [], [IntrNoMem]>;
 
Index: llvm/include/llvm/IR/IRBuilder.h

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

[Chuanqi Xu via Phabricator via cfe-commits]

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 still treat a call to the intrinsic as 
> readnone.

Yeah, the point here is that we used operand bundle in `D125292`. The operand 
bundles would break the logic you said, see: 
https://github.com/llvm/llvm-project/blob/3b762b3ab8d205cd6a7d42c96d39d5f4f701f2ab/llvm/include/llvm/IR/InstrTypes.h#L2315-L2325.

> I think I'd prefer to lower the intrinsic as part of the codegen optimization 
> pipeline, not the optimization pipeline.  So maybe just in in SelectionDAG?  
> I mean, it doesn't matter much for your planned usage in coroutines, but it's 
> more similar to other intrinsics, and more like what we expect it to look 
> like in the future.

Good idea. Would do.


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

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

[Eli Friedman via Phabricator via cfe-commits]

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 intrinsic as part of the codegen optimization 
pipeline, not the optimization pipeline.  So maybe just in in SelectionDAG?  I 
mean, it doesn't matter much for your planned usage in coroutines, but it's 
more similar to other intrinsics, and more like what we expect it to look like 
in the future.


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

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

[Chuanqi Xu via Phabricator via cfe-commits]

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