[PATCH] D115456: Implement on-demand TLS initialization for Microsoft CXX ABI

[David Majnemer via Phabricator via cfe-commits]

majnemer accepted this revision.
majnemer added a comment.
This revision is now accepted and ready to land.

Looks great! Please give others some time to review it as it is a holiday 
season...


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

https://reviews.llvm.org/D115456

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

[PATCH] D115456: Implement on-demand TLS initialization for Microsoft CXX ABI

[Maurice Heumann via Phabricator via cfe-commits]

momo5502 updated this revision to Diff 397107.
momo5502 added a comment.

In D115456#3217857 , @majnemer wrote:

> In D115456#3217595 , @momo5502 
> wrote:
>
>> In D115456#3216811 , @majnemer 
>> wrote:
>>
>>> This is looking great! Just a few more questions.
>>>
>>> What is the behavior with something like:
>>>
>>>   thread_local int x = 2;
>>>   int f() {
>>> return x;
>>>   }
>>>
>>> I'm wondering if we need to move this logic 
>>> 
>>>  into the generic C++ ABI implementation.
>>
>> The MS compiler only emits the dynamic initializers for variables with 
>> constructors/destructors, just like it is currently done here for the 
>> Itanium ABI.
>> I also thought about adopting that behaviour, but I think threre are 
>> edge-cases when triggering dynamic TLS initialization even for constant 
>> variables is useful.
>> For example there might be custom TLS callbacks that can affect the value of 
>> this variable.
>>
>> If desired, I can change it to match the behaviour of MS, but I thought it 
>> could be beneficial to diverge in this case.
>
> IMO, it is probably best to match behavior here within reason (ignoring bugs 
> latent in MSVC) here. My thinking is that in the face of COMDATs, we cannot 
> ensure which copy of an inline function will make its way to the binary and 
> different link orders would provide different behavior.

Fair enough. Logic has now moved to the generic C++ ABI implementation and the 
test was adjusted.


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

https://reviews.llvm.org/D115456

Files:
  clang/include/clang/Basic/LangOptions.h
  clang/lib/CodeGen/CGCXXABI.cpp
  clang/lib/CodeGen/CGCXXABI.h
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  clang/lib/CodeGen/MicrosoftCXXABI.cpp
  clang/test/CodeGenCXX/ms-thread_local.cpp

Index: clang/test/CodeGenCXX/ms-thread_local.cpp
===
--- clang/test/CodeGenCXX/ms-thread_local.cpp
+++ clang/test/CodeGenCXX/ms-thread_local.cpp
@@ -1,5 +1,6 @@
-// RUN: %clang_cc1 %s -std=c++1y -triple=i686-pc-win32 -emit-llvm -o - | FileCheck %s
-// RUN: %clang_cc1 %s  -std=c++1y -triple=i686-pc-win32 -ftls-model=local-dynamic -emit-llvm -o - | FileCheck %s -check-prefix=CHECK-LD
+// RUN: %clang_cc1 %s -std=c++1y -triple=i686-pc-win32 -fms-compatibility-version=19.25 -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 %s -std=c++1y -triple=i686-pc-win32 -fms-compatibility-version=19.20 -emit-llvm -o - | FileCheck %s -check-prefix=CHECK-LEGACY
+// RUN: %clang_cc1 %s -std=c++1y -triple=i686-pc-win32 -fms-compatibility-version=19.25 -ftls-model=local-dynamic -emit-llvm -o - | FileCheck %s -check-prefix=CHECK-LD
 
 struct A {
   A();
@@ -17,14 +18,22 @@
 
 // CHECK-DAG: @"?b@@3UA@@A" = dso_local thread_local global %struct.A zeroinitializer, align 1
 // CHECK-DAG: @"__tls_init$initializer$" = internal constant void ()* @__tls_init, section ".CRT$XDU"
+// CHECK-DAG: @__tls_guard = external dso_local thread_local global i8
 // CHECK-LD-DAG: @"?b@@3UA@@A" = dso_local thread_local(localdynamic) global %struct.A zeroinitializer, align 1
 // CHECK-LD-DAG: @"__tls_init$initializer$" = internal constant void ()* @__tls_init, section ".CRT$XDU"
+// CHECK-LD-DAG: @__tls_guard = external dso_local thread_local global i8
+// CHECK-LEGACY-NOT: @__tls_guard = external dso_local thread_local global i8
 thread_local A b;
 
-// CHECK-LABEL: define internal void @__tls_init()
-// CHECK: call void @"??__Eb@@YAXXZ"
-// CHECK-LD-LABEL: define internal void @__tls_init()
-// CHECK-LD: call void @"??__Eb@@YAXXZ"
+// CHECK-LABEL: declare dso_local void @__dyn_tls_on_demand_init()
+// CHECK-LD-LABEL: declare dso_local void @__dyn_tls_on_demand_init()
+// CHECK-LEGACY-NOT: declare dso_local void @__dyn_tls_on_demand_init()
+
+// CHECK-LABEL: define dso_local void @"?f@@YA?AUA@@XZ"(%struct.A* noalias sret(%struct.A) align 1 %agg.result)
+// CHECK: call void @__dyn_tls_on_demand_init()
+// CHECK-LD-LABEL: define dso_local void @"?f@@YA?AUA@@XZ"(%struct.A* noalias sret(%struct.A) align 1 %agg.result)
+// CHECK-LD: call void @__dyn_tls_on_demand_init()
+// CHECK-LEGACY-NOT: call void @__dyn_tls_on_demand_init()
 
 thread_local A &c = b;
 thread_local A &d = c;
@@ -35,6 +44,22 @@
   return c;
 }
 
+// CHECK-LABEL: define dso_local i32 @"?g@@YAHXZ"()
+// CHECK-NOT: call void @__dyn_tls_on_demand_init()
+// CHECK-LD-LABEL: define dso_local i32 @"?g@@YAHXZ"()
+// CHECK-LD-NOT: call void @__dyn_tls_on_demand_init()
+
+thread_local int e = 2;
+
+int g() {
+  return e;
+}
+
+// CHECK-LABEL: define internal void @__tls_init()
+// CHECK: call void @"??__Eb@@YAXXZ"
+// CHECK-LD-LABEL: define internal void @__tls_init()
+// CHECK-LD: call void @"??__Eb@@YAXXZ"
+
 // CHECK: 

[PATCH] D115456: Implement on-demand TLS initialization for Microsoft CXX ABI

[David Majnemer via Phabricator via cfe-commits]

majnemer added a comment.

In D115456#3217595 , @momo5502 wrote:

> In D115456#3216811 , @majnemer 
> wrote:
>
>> This is looking great! Just a few more questions.
>>
>> What is the behavior with something like:
>>
>>   thread_local int x = 2;
>>   int f() {
>> return x;
>>   }
>>
>> I'm wondering if we need to move this logic 
>> 
>>  into the generic C++ ABI implementation.
>
> The MS compiler only emits the dynamic initializers for variables with 
> constructors/destructors, just like it is currently done here for the Itanium 
> ABI.
> I also thought about adopting that behaviour, but I think threre are 
> edge-cases when triggering dynamic TLS initialization even for constant 
> variables is useful.
> For example there might be custom TLS callbacks that can affect the value of 
> this variable.
>
> If desired, I can change it to match the behaviour of MS, but I thought it 
> could be beneficial to diverge in this case.

IMO, it is probably best to match behavior here within reason (ignoring bugs 
latent in MSVC) here. My thinking is that in the face of COMDATs, we cannot 
ensure which copy of an inline function will make its way to the binary and 
different link orders would provide different behavior.


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

https://reviews.llvm.org/D115456

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

[PATCH] D115456: Implement on-demand TLS initialization for Microsoft CXX ABI

[Maurice Heumann via Phabricator via cfe-commits]

momo5502 added a comment.

In D115456#3216811 , @majnemer wrote:

> This is looking great! Just a few more questions.
>
> What is the behavior with something like:
>
>   thread_local int x = 2;
>   int f() {
> return x;
>   }
>
> I'm wondering if we need to move this logic 
> 
>  into the generic C++ ABI implementation.

The MS compiler only emits the dynamic initializers for variables with 
constructors/destructors, just like it is currently done here for the Itanium 
ABI.
I also thought about adopting that behaviour, but I think threre are edge-cases 
when triggering dynamic TLS initialization even for constant variables is 
useful.
For example there might be custom TLS callbacks that can affect the value of 
this variable.

If desired, I can change it to match the behaviour of MS, but I thought it 
could be beneficial to diverge in this case.


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

https://reviews.llvm.org/D115456

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

[PATCH] D115456: Implement on-demand TLS initialization for Microsoft CXX ABI

[David Majnemer via Phabricator via cfe-commits]

majnemer added a comment.

This is looking great! Just a few more questions.

What is the behavior with something like:

  thread_local int x = 2;
  int f() {
return x;
  }

I'm wondering if we need to move this logic 

 into the generic C++ ABI implementation.


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

https://reviews.llvm.org/D115456

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

[PATCH] D115456: Implement on-demand TLS initialization for Microsoft CXX ABI

[Maurice Heumann via Phabricator via cfe-commits]

momo5502 updated this revision to Diff 396783.
momo5502 marked 4 inline comments as done.
momo5502 added a comment.

Comments were addressed


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

https://reviews.llvm.org/D115456

Files:
  clang/include/clang/Basic/LangOptions.h
  clang/lib/CodeGen/MicrosoftCXXABI.cpp
  clang/test/CodeGenCXX/ms-thread_local.cpp

Index: clang/test/CodeGenCXX/ms-thread_local.cpp
===
--- clang/test/CodeGenCXX/ms-thread_local.cpp
+++ clang/test/CodeGenCXX/ms-thread_local.cpp
@@ -1,5 +1,6 @@
-// RUN: %clang_cc1 %s -std=c++1y -triple=i686-pc-win32 -emit-llvm -o - | FileCheck %s
-// RUN: %clang_cc1 %s  -std=c++1y -triple=i686-pc-win32 -ftls-model=local-dynamic -emit-llvm -o - | FileCheck %s -check-prefix=CHECK-LD
+// RUN: %clang_cc1 %s -std=c++1y -triple=i686-pc-win32 -fms-compatibility-version=19.25 -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 %s -std=c++1y -triple=i686-pc-win32 -fms-compatibility-version=19.20 -emit-llvm -o - | FileCheck %s -check-prefix=CHECK-LEGACY
+// RUN: %clang_cc1 %s -std=c++1y -triple=i686-pc-win32 -fms-compatibility-version=19.25 -ftls-model=local-dynamic -emit-llvm -o - | FileCheck %s -check-prefix=CHECK-LD
 
 struct A {
   A();
@@ -17,10 +18,23 @@
 
 // CHECK-DAG: @"?b@@3UA@@A" = dso_local thread_local global %struct.A zeroinitializer, align 1
 // CHECK-DAG: @"__tls_init$initializer$" = internal constant void ()* @__tls_init, section ".CRT$XDU"
+// CHECK-DAG: @__tls_guard = external dso_local thread_local global i8
 // CHECK-LD-DAG: @"?b@@3UA@@A" = dso_local thread_local(localdynamic) global %struct.A zeroinitializer, align 1
 // CHECK-LD-DAG: @"__tls_init$initializer$" = internal constant void ()* @__tls_init, section ".CRT$XDU"
+// CHECK-LD-DAG: @__tls_guard = external dso_local thread_local global i8
+// CHECK-LEGACY-NOT: @__tls_guard = external dso_local thread_local global i8
 thread_local A b;
 
+// CHECK-LABEL: declare dso_local void @__dyn_tls_on_demand_init()
+// CHECK-LD-LABEL: declare dso_local void @__dyn_tls_on_demand_init()
+// CHECK-LEGACY-NOT: declare dso_local void @__dyn_tls_on_demand_init()
+
+// CHECK-LABEL: define dso_local void @"?f@@YA?AUA@@XZ"(%struct.A* noalias sret(%struct.A) align 1 %agg.result)
+// CHECK: call void @__dyn_tls_on_demand_init()
+// CHECK-LD-LABEL: define dso_local void @"?f@@YA?AUA@@XZ"(%struct.A* noalias sret(%struct.A) align 1 %agg.result)
+// CHECK-LD: call void @__dyn_tls_on_demand_init()
+// CHECK-LEGACY-NOT: call void @__dyn_tls_on_demand_init()
+
 // CHECK-LABEL: define internal void @__tls_init()
 // CHECK: call void @"??__Eb@@YAXXZ"
 // CHECK-LD-LABEL: define internal void @__tls_init()
Index: clang/lib/CodeGen/MicrosoftCXXABI.cpp
===
--- clang/lib/CodeGen/MicrosoftCXXABI.cpp
+++ clang/lib/CodeGen/MicrosoftCXXABI.cpp
@@ -401,7 +401,8 @@
   ArrayRef CXXThreadLocalInitVars) override;
 
   bool usesThreadWrapperFunction(const VarDecl *VD) const override {
-return false;
+return getContext().getLangOpts().isCompatibleWithMSVC(
+LangOptions::MSVC2019_5);
   }
   LValue EmitThreadLocalVarDeclLValue(CodeGenFunction &CGF, const VarDecl *VD,
   QualType LValType) override;
@@ -2397,11 +2398,97 @@
   }
 }
 
+static llvm::GlobalValue *getTlsGuardVar(CodeGenModule &CGM) {
+  // __tls_guard comes from the MSVC runtime and reflects
+  // whether TLS has been initialized for a particular thread.
+  // It is set from within __dyn_tls_init by the runtime.
+  // Every library and executable has its own variable.
+  llvm::Type *VTy = llvm::Type::getInt8Ty(CGM.getLLVMContext());
+  llvm::Constant *TlsGuardConstant =
+  CGM.CreateRuntimeVariable(VTy, "__tls_guard");
+  llvm::GlobalValue *TlsGuard = cast(TlsGuardConstant);
+
+  TlsGuard->setThreadLocal(true);
+
+  return TlsGuard;
+}
+
+static llvm::FunctionCallee getDynTlsOnDemandInitFn(CodeGenModule &CGM) {
+  // __dyn_tls_on_demand_init comes from the MSVC runtime and triggers
+  // dynamic TLS initialization by calling __dyn_tls_init internally.
+  llvm::FunctionType *FTy =
+  llvm::FunctionType::get(llvm::Type::getVoidTy(CGM.getLLVMContext()), {},
+  /*isVarArg=*/false);
+  return CGM.CreateRuntimeFunction(
+  FTy, "__dyn_tls_on_demand_init",
+  llvm::AttributeList::get(CGM.getLLVMContext(),
+   llvm::AttributeList::FunctionIndex,
+   llvm::Attribute::NoUnwind),
+  /*Local=*/true);
+}
+
+static void emitTlsGuardCheck(CodeGenFunction &CGF, llvm::GlobalValue *TlsGuard,
+  llvm::BasicBlock *DynInitBB,
+  llvm::BasicBlock *ContinueBB) {
+  llvm::LoadInst *TlsGuardValue =
+  CGF.Builder.CreateLoad(Address(TlsGuard, CharUnits::One()));
+  llvm::Value *CmpResult =
+  CGF.Builder.CreateICmpEQ(TlsGuardValue, CGF

[PATCH] D115456: Implement on-demand TLS initialization for Microsoft CXX ABI

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added inline comments.



Comment at: clang/lib/CodeGen/MicrosoftCXXABI.cpp:2470
+// Dynamic TLS initialization works by checking the state of a
+// guard varibale (__tls_guard) to see whether TLS initialization
+// for a thread has happend yet.

s/varibale/variable/



Comment at: clang/lib/CodeGen/MicrosoftCXXABI.cpp:2477
+
+  // Emit the variable just like any regular global variable
+

Missing period at the end of the sentence please.



Comment at: clang/test/CodeGenCXX/ms-thread_local.cpp:1
-// RUN: %clang_cc1 %s -std=c++1y -triple=i686-pc-win32 -emit-llvm -o - | 
FileCheck %s
-// RUN: %clang_cc1 %s  -std=c++1y -triple=i686-pc-win32 
-ftls-model=local-dynamic -emit-llvm -o - | FileCheck %s -check-prefix=CHECK-LD
+// RUN: %clang_cc1 %s -std=c++1y -triple=i686-pc-win32 
-fms-compatibility-version=1925 -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 %s -std=c++1y -triple=i686-pc-win32 
-fms-compatibility-version=1925 -ftls-model=local-dynamic -emit-llvm -o - | 
FileCheck %s -check-prefix=CHECK-LD

Also please check with `-fms-compatibility-version=1920` to ensure that the 
dynamic initialization isn't generated.


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

https://reviews.llvm.org/D115456

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

[PATCH] D115456: Implement on-demand TLS initialization for Microsoft CXX ABI

[David Majnemer via Phabricator via cfe-commits]

majnemer added inline comments.



Comment at: clang/lib/CodeGen/MicrosoftCXXABI.cpp:404
   bool usesThreadWrapperFunction(const VarDecl *VD) const override {
-return false;
+return true;
   }

Should this depend on the MSVC compatibility version?


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

https://reviews.llvm.org/D115456

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

[PATCH] D115456: Implement on-demand TLS initialization for Microsoft CXX ABI

[Maurice Heumann via Phabricator via cfe-commits]

momo5502 updated this revision to Diff 395856.
momo5502 edited the summary of this revision.
momo5502 added a comment.
Herald added a subscriber: dexonsmith.

A call to `isCompatibleWithMSVC` was added with the proper version that 
introduced the change (1925) and comments describing the intention of the 
change were added.

Additionally, a bug was fixed. Setting the guard variable to true before doing 
the initialization breaks the feature, as the guard needs to be false for 
`__dyn_tls_init` to work.
Not emitting the store to the guard should be fine, as it will be set by 
`__dyn_tls_init` anyways. The behaviour is now identical to the way Microsoft's 
compiler does it.


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

https://reviews.llvm.org/D115456

Files:
  clang/include/clang/Basic/LangOptions.h
  clang/lib/CodeGen/MicrosoftCXXABI.cpp
  clang/test/CodeGenCXX/ms-thread_local.cpp

Index: clang/test/CodeGenCXX/ms-thread_local.cpp
===
--- clang/test/CodeGenCXX/ms-thread_local.cpp
+++ clang/test/CodeGenCXX/ms-thread_local.cpp
@@ -1,5 +1,5 @@
-// RUN: %clang_cc1 %s -std=c++1y -triple=i686-pc-win32 -emit-llvm -o - | FileCheck %s
-// RUN: %clang_cc1 %s  -std=c++1y -triple=i686-pc-win32 -ftls-model=local-dynamic -emit-llvm -o - | FileCheck %s -check-prefix=CHECK-LD
+// RUN: %clang_cc1 %s -std=c++1y -triple=i686-pc-win32 -fms-compatibility-version=1925 -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 %s -std=c++1y -triple=i686-pc-win32 -fms-compatibility-version=1925 -ftls-model=local-dynamic -emit-llvm -o - | FileCheck %s -check-prefix=CHECK-LD
 
 struct A {
   A();
@@ -17,10 +17,20 @@
 
 // CHECK-DAG: @"?b@@3UA@@A" = dso_local thread_local global %struct.A zeroinitializer, align 1
 // CHECK-DAG: @"__tls_init$initializer$" = internal constant void ()* @__tls_init, section ".CRT$XDU"
+// CHECK-DAG: @__tls_guard = external dso_local thread_local global i8
 // CHECK-LD-DAG: @"?b@@3UA@@A" = dso_local thread_local(localdynamic) global %struct.A zeroinitializer, align 1
 // CHECK-LD-DAG: @"__tls_init$initializer$" = internal constant void ()* @__tls_init, section ".CRT$XDU"
+// CHECK-LD-DAG: @__tls_guard = external dso_local thread_local global i8
 thread_local A b;
 
+// CHECK-LABEL: declare dso_local void @__dyn_tls_on_demand_init()
+// CHECK-LD-LABEL: declare dso_local void @__dyn_tls_on_demand_init()
+
+// CHECK-LABEL: define dso_local void @"?f@@YA?AUA@@XZ"(%struct.A* noalias sret(%struct.A) align 1 %agg.result)
+// CHECK: call void @__dyn_tls_on_demand_init()
+// CHECK-LD-LABEL: define dso_local void @"?f@@YA?AUA@@XZ"(%struct.A* noalias sret(%struct.A) align 1 %agg.result)
+// CHECK-LD: call void @__dyn_tls_on_demand_init()
+
 // CHECK-LABEL: define internal void @__tls_init()
 // CHECK: call void @"??__Eb@@YAXXZ"
 // CHECK-LD-LABEL: define internal void @__tls_init()
Index: clang/lib/CodeGen/MicrosoftCXXABI.cpp
===
--- clang/lib/CodeGen/MicrosoftCXXABI.cpp
+++ clang/lib/CodeGen/MicrosoftCXXABI.cpp
@@ -401,7 +401,7 @@
   ArrayRef CXXThreadLocalInitVars) override;
 
   bool usesThreadWrapperFunction(const VarDecl *VD) const override {
-return false;
+return true;
   }
   LValue EmitThreadLocalVarDeclLValue(CodeGenFunction &CGF, const VarDecl *VD,
   QualType LValType) override;
@@ -2397,11 +2397,99 @@
   }
 }
 
+static llvm::GlobalValue *getTlsGuardVar(CodeGenModule &CGM) {
+  // __tls_guard comes from the MSVC runtime and reflects
+  // whether TLS has been initialized for a particular thread.
+  // It is set from within __dyn_tls_init by the runtime.
+  // Every library and executable has its own variable.
+  llvm::Type *VTy = llvm::Type::getInt8Ty(CGM.getLLVMContext());
+  llvm::Constant *TlsGuardConstant =
+  CGM.CreateRuntimeVariable(VTy, "__tls_guard");
+  llvm::GlobalValue *TlsGuard = cast(TlsGuardConstant);
+
+  TlsGuard->setThreadLocal(true);
+
+  return TlsGuard;
+}
+
+static llvm::FunctionCallee getDynTlsOnDemandInitFn(CodeGenModule &CGM) {
+  // __dyn_tls_on_demand_init comes from the MSVC runtime and triggers
+  // dynamic TLS initialization by calling __dyn_tls_init internally.
+  llvm::FunctionType *FTy =
+  llvm::FunctionType::get(llvm::Type::getVoidTy(CGM.getLLVMContext()), {},
+  /*isVarArg=*/false);
+  return CGM.CreateRuntimeFunction(
+  FTy, "__dyn_tls_on_demand_init",
+  llvm::AttributeList::get(CGM.getLLVMContext(),
+   llvm::AttributeList::FunctionIndex,
+   llvm::Attribute::NoUnwind),
+  /*Local=*/true);
+}
+
+static void emitTlsGuardCheck(CodeGenFunction &CGF, llvm::GlobalValue *TlsGuard,
+  llvm::BasicBlock *DynInitBB,
+  llvm::BasicBlock *ContinueBB) {
+  llvm::LoadInst *TlsGuardValue =
+  CGF.Builder.CreateLoad(Address(TlsGuar

[PATCH] D115456: Implement on-demand TLS initialization for Microsoft CXX ABI

[David Majnemer via Phabricator via cfe-commits]

majnemer added a comment.

Is this a new feature in MSVC? Seems like it might 

 be. If so, should it be predicated on `isCompatibleWithMSVC(1925)` or 
something?


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

https://reviews.llvm.org/D115456

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

[PATCH] D115456: Implement on-demand TLS initialization for Microsoft CXX ABI

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

I probably don't have time to review this, so let me redirect to @hans.




Comment at: clang/lib/CodeGen/MicrosoftCXXABI.cpp:2400
 
+static llvm::GlobalValue *getTlsGuardVar(CodeGenModule &CGM) {
+  llvm::Type *VTy = llvm::Type::getInt8Ty(CGM.getLLVMContext());

My understanding is that every DLL has exactly one `__tls_guard` variable. All 
TLS variables in a DLL are initialized as soon as a thread accesses one TLS 
variable for a DLL. Is that accurate? Can you add comments about the way this 
is intended to work here?


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

https://reviews.llvm.org/D115456

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

[PATCH] D115456: Implement on-demand TLS initialization for Microsoft CXX ABI

[Maurice Heumann via Phabricator via cfe-commits]

momo5502 updated this revision to Diff 393247.

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

https://reviews.llvm.org/D115456

Files:
  clang/lib/CodeGen/MicrosoftCXXABI.cpp
  clang/test/CodeGenCXX/ms-thread_local.cpp

Index: clang/test/CodeGenCXX/ms-thread_local.cpp
===
--- clang/test/CodeGenCXX/ms-thread_local.cpp
+++ clang/test/CodeGenCXX/ms-thread_local.cpp
@@ -17,10 +17,20 @@
 
 // CHECK-DAG: @"?b@@3UA@@A" = dso_local thread_local global %struct.A zeroinitializer, align 1
 // CHECK-DAG: @"__tls_init$initializer$" = internal constant void ()* @__tls_init, section ".CRT$XDU"
+// CHECK-DAG: @__tls_guard = external dso_local thread_local global i8
 // CHECK-LD-DAG: @"?b@@3UA@@A" = dso_local thread_local(localdynamic) global %struct.A zeroinitializer, align 1
 // CHECK-LD-DAG: @"__tls_init$initializer$" = internal constant void ()* @__tls_init, section ".CRT$XDU"
+// CHECK-LD-DAG: @__tls_guard = external dso_local thread_local global i8
 thread_local A b;
 
+// CHECK-LABEL: declare dso_local void @__dyn_tls_on_demand_init()
+// CHECK-LD-LABEL: declare dso_local void @__dyn_tls_on_demand_init()
+
+// CHECK-LABEL: define dso_local void @"?f@@YA?AUA@@XZ"(%struct.A* noalias sret(%struct.A) align 1 %agg.result)
+// CHECK: call void @__dyn_tls_on_demand_init()
+// CHECK-LD-LABEL: define dso_local void @"?f@@YA?AUA@@XZ"(%struct.A* noalias sret(%struct.A) align 1 %agg.result)
+// CHECK-LD: call void @__dyn_tls_on_demand_init()
+
 // CHECK-LABEL: define internal void @__tls_init()
 // CHECK: call void @"??__Eb@@YAXXZ"
 // CHECK-LD-LABEL: define internal void @__tls_init()
Index: clang/lib/CodeGen/MicrosoftCXXABI.cpp
===
--- clang/lib/CodeGen/MicrosoftCXXABI.cpp
+++ clang/lib/CodeGen/MicrosoftCXXABI.cpp
@@ -401,7 +401,7 @@
   ArrayRef CXXThreadLocalInitVars) override;
 
   bool usesThreadWrapperFunction(const VarDecl *VD) const override {
-return false;
+return true;
   }
   LValue EmitThreadLocalVarDeclLValue(CodeGenFunction &CGF, const VarDecl *VD,
   QualType LValType) override;
@@ -2397,11 +2397,83 @@
   }
 }
 
+static llvm::GlobalValue *getTlsGuardVar(CodeGenModule &CGM) {
+  llvm::Type *VTy = llvm::Type::getInt8Ty(CGM.getLLVMContext());
+  llvm::Constant *TlsGuardConstant =
+  CGM.CreateRuntimeVariable(VTy, "__tls_guard");
+  llvm::GlobalValue *TlsGuard = cast(TlsGuardConstant);
+
+  TlsGuard->setThreadLocal(true);
+
+  return TlsGuard;
+}
+
+static llvm::FunctionCallee getDynTlsOnDemandInitFn(CodeGenModule &CGM) {
+  llvm::FunctionType *FTy =
+  llvm::FunctionType::get(llvm::Type::getVoidTy(CGM.getLLVMContext()), {},
+  /*isVarArg=*/false);
+  return CGM.CreateRuntimeFunction(
+  FTy, "__dyn_tls_on_demand_init",
+  llvm::AttributeList::get(CGM.getLLVMContext(),
+   llvm::AttributeList::FunctionIndex,
+   llvm::Attribute::NoUnwind),
+  /*Local=*/true);
+}
+
+static void emitTlsGuardCheck(CodeGenFunction &CGF, llvm::GlobalValue *TlsGuard,
+  llvm::BasicBlock *DynInitBB,
+  llvm::BasicBlock *ContinueBB) {
+  llvm::LoadInst *TlsGuardValue =
+  CGF.Builder.CreateLoad(Address(TlsGuard, CharUnits::One()));
+  llvm::Value *CmpResult =
+  CGF.Builder.CreateICmpEQ(TlsGuardValue, CGF.Builder.getInt8(0));
+  CGF.Builder.CreateCondBr(CmpResult, DynInitBB, ContinueBB);
+}
+
+static void emitDynamicTlsInitialization(CodeGenFunction &CGF,
+ llvm::GlobalValue *TlsGuard,
+ llvm::BasicBlock *ContinueBB) {
+  CGF.Builder.CreateStore(CGF.Builder.getInt8(1),
+  Address(TlsGuard, CharUnits::One()));
+
+  llvm::FunctionCallee Initializer = getDynTlsOnDemandInitFn(CGF.CGM);
+  llvm::Function *InitializerFunction =
+  cast(Initializer.getCallee());
+  llvm::CallInst *CallVal = CGF.Builder.CreateCall(InitializerFunction);
+  CallVal->setCallingConv(InitializerFunction->getCallingConv());
+
+  CGF.Builder.CreateBr(ContinueBB);
+}
+
 LValue MicrosoftCXXABI::EmitThreadLocalVarDeclLValue(CodeGenFunction &CGF,
  const VarDecl *VD,
  QualType LValType) {
-  CGF.CGM.ErrorUnsupported(VD, "thread wrappers");
-  return LValue();
+  llvm::BasicBlock *DynInitBB =
+  CGF.createBasicBlock("dyntls.dyn_init", CGF.CurFn);
+  llvm::BasicBlock *ContinueBB =
+  CGF.createBasicBlock("dyntls.continue", CGF.CurFn);
+
+  llvm::GlobalValue *TlsGuard = getTlsGuardVar(CGF.CGM);
+
+  emitTlsGuardCheck(CGF, TlsGuard, DynInitBB, ContinueBB);
+  CGF.Builder.SetInsertPoint(DynInitBB);
+  emitDynamicTlsInitialization(CGF, TlsGuard, ContinueBB);
+  CGF.Builder.SetInsertPoint(ContinueBB);
+
+  l

[PATCH] D115456: Implement on-demand TLS initialization for Microsoft CXX ABI

[Maurice Heumann via Phabricator via cfe-commits]

momo5502 created this revision.
momo5502 added reviewers: rnk, eugenis.
momo5502 added a project: clang.
momo5502 requested review of this revision.
Herald added a subscriber: cfe-commits.

TLS initializers, for example constructors of thread-local variables, don't 
necessarily get called. If for example a thread was created before a module is 
loaded, the module's TLS initializers are not executed for this particular 
thread.

This is why Microsoft added support for dynamic TLS initialization. Before 
every use of thread-local variables, a check is added that runs the module's 
TLS initializers on-demand.

To do this, the method `__dyn_tls_on_demand_init` gets called. Internally, it 
simply calls `__dyn_tls_init`.
To prevent calling initializers again, a guard variable called `__tls_guard` is 
checked and set before executing the call.

No additional TLS initializer that sets the guard needs to be emitted, as the 
guard always gets set by `__dyn_tls_init`.
The guard is also checked again within `__dyn_tls_init`. This makes our check 
redundant, however, as Microsoft's compiler also emits this check, the 
behaviour is adopted here.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D115456

Files:
  clang/lib/CodeGen/MicrosoftCXXABI.cpp
  clang/test/CodeGenCXX/ms-thread_local.cpp

Index: clang/test/CodeGenCXX/ms-thread_local.cpp
===
--- clang/test/CodeGenCXX/ms-thread_local.cpp
+++ clang/test/CodeGenCXX/ms-thread_local.cpp
@@ -17,10 +17,20 @@
 
 // CHECK-DAG: @"?b@@3UA@@A" = dso_local thread_local global %struct.A zeroinitializer, align 1
 // CHECK-DAG: @"__tls_init$initializer$" = internal constant void ()* @__tls_init, section ".CRT$XDU"
+// CHECK-DAG: @__tls_guard = external dso_local thread_local global i8
 // CHECK-LD-DAG: @"?b@@3UA@@A" = dso_local thread_local(localdynamic) global %struct.A zeroinitializer, align 1
 // CHECK-LD-DAG: @"__tls_init$initializer$" = internal constant void ()* @__tls_init, section ".CRT$XDU"
+// CHECK-LD-DAG: @__tls_guard = external dso_local thread_local global i8
 thread_local A b;
 
+// CHECK-LABEL: declare dso_local void @__dyn_tls_on_demand_init()
+// CHECK-LD-LABEL: declare dso_local void @__dyn_tls_on_demand_init()
+
+// CHECK-LABEL: define dso_local void @"?f@@YA?AUA@@XZ"(%struct.A* noalias sret(%struct.A) align 1 %agg.result)
+// CHECK: call void @__dyn_tls_on_demand_init()
+// CHECK-LD-LABEL: define dso_local void @"?f@@YA?AUA@@XZ"(%struct.A* noalias sret(%struct.A) align 1 %agg.result)
+// CHECK-LD: call void @__dyn_tls_on_demand_init()
+
 // CHECK-LABEL: define internal void @__tls_init()
 // CHECK: call void @"??__Eb@@YAXXZ"
 // CHECK-LD-LABEL: define internal void @__tls_init()
Index: clang/lib/CodeGen/MicrosoftCXXABI.cpp
===
--- clang/lib/CodeGen/MicrosoftCXXABI.cpp
+++ clang/lib/CodeGen/MicrosoftCXXABI.cpp
@@ -401,7 +401,7 @@
   ArrayRef CXXThreadLocalInitVars) override;
 
   bool usesThreadWrapperFunction(const VarDecl *VD) const override {
-return false;
+return true;
   }
   LValue EmitThreadLocalVarDeclLValue(CodeGenFunction &CGF, const VarDecl *VD,
   QualType LValType) override;
@@ -2397,11 +2397,87 @@
   }
 }
 
+static llvm::GlobalValue *getTlsGuardVar(CodeGenModule &CGM) {
+  llvm::Type *VTy = llvm::Type::getInt8Ty(CGM.getLLVMContext());
+  llvm::Constant *TlsGuardConstant =
+  CGM.CreateRuntimeVariable(VTy, "__tls_guard");
+  llvm::GlobalValue *TlsGuard = cast(TlsGuardConstant);
+
+  TlsGuard->setThreadLocal(true);
+
+  return TlsGuard;
+}
+
+static llvm::FunctionCallee getDynTlsOnDemandInitFn(CodeGenModule &CGM) {
+  llvm::FunctionType *FTy =
+  llvm::FunctionType::get(llvm::Type::getVoidTy(CGM.getLLVMContext()), {},
+  /*isVarArg=*/false);
+  return CGM.CreateRuntimeFunction(
+  FTy, "__dyn_tls_on_demand_init",
+  llvm::AttributeList::get(CGM.getLLVMContext(),
+   llvm::AttributeList::FunctionIndex,
+   llvm::Attribute::NoUnwind),
+  /*Local=*/true);
+}
+
+static void emitTlsGuardCheck(CodeGenFunction &CGF, llvm::GlobalValue *TlsGuard,
+  llvm::BasicBlock *DynInitBB,
+  llvm::BasicBlock *ContinueBB) {
+  llvm::LoadInst *TlsGuardValue =
+  CGF.Builder.CreateLoad(Address(TlsGuard, CharUnits::One()));
+  llvm::Value *CmpResult =
+  CGF.Builder.CreateICmpEQ(TlsGuardValue, CGF.Builder.getInt8(0));
+  CGF.Builder.CreateCondBr(CmpResult, DynInitBB, ContinueBB);
+}
+
+static void emitDynamicTlsInitialization(CodeGenFunction &CGF,
+ llvm::GlobalValue *TlsGuard,
+ llvm::BasicBlock *ContinueBB) {
+  CGF.Builder.CreateStore(CGF.Builder.getInt8(1),
+  Address(TlsGuard, CharUnits::One()));
+
+  llvm::Funct