[WebAssembly] Use `localexec` as default TLS model for non-Emscripten targets

[abrown]

Summary

Only Emscripten supports dynamic linking with threads. To use thread-local
storage for other targets, this change defaults to the localexec model.

[sbc100]

lgtm! Although we probably need to test.. perhaps there already is one which needs updating due to this test? I imagine it would be in the test/CodeGen/WebAssembly directory.

[sunfish]

LGTM too. For testing, I think it would work to make a copy of llvm/test/CodeGen/WebAssembly/tls-local-exec.ll and change its thread_local(localexec)s to thread_locals.

[abrown]

This comment was removed by abrown.

[abrown]

This comment was removed by abrown.

[abrown]

This comment was removed by abrown.

[abrown]

This comment was removed by abrown.

[abrown]

[WebAssembly] Use localexec as default TLS model for non-Emscripten targets

Only Emscripten supports dynamic linking with threads. To use thread-local
storage for other targets, this change defaults to the localexec model.

Additionally, this changes the tests slightly:

• Ignore non-Emscripten test cases for general dynamic TLS model
• Use default thread-local model (i.e., localexec)

llvm/test/CodeGen/WebAssembly/tls-general-dynamic.ll

2

@sbc100, @sunfish: I am not exactly sure what to do here. Ideally it would be nice to test that the "no-Emscripten, general dynamic, no DSO local case" actually emits the same code as localexec, but this test does not seem set up for this.

[sunfish]

llvm/test/CodeGen/WebAssembly/tls-general-dynamic.ll

2

I suggest leaving this test as-is, because updating it to handle non-Emscripten targets would require adding all new CHECK lines, which would be a lot of clutter. I suggest just adding RUN lines to tls-initial-exec.ll, or perhaps making a copy of that file, to test this feature.

[penzn]

llvm/test/CodeGen/WebAssembly/tls-general-dynamic.ll

2

The test would fail if these two lines are preserved, because they check that default behavior is to throw an error.

[sunfish]

llvm/test/CodeGen/WebAssembly/tls-general-dynamic.ll

2

Ah, right. Can we just disable non-Emscripten targets here? My main thought here is to avoid needing to add all the CHECKs for local-exec code in tls-general-dynamic.ll.

[sbc100]

llvm/test/CodeGen/WebAssembly/tls-local-exec.ll

101

Shouldn't we leave this file as is, with the explicit TLS model? (according the language reference the default thread model is general dynamic: https://llvm.org/docs/LangRef.html#thread-local-storage-models "If no explicit model is given, the “general dynamic” model is used.").

I guess you are trying to test here that general dynamic is treated "as if" its local-exec, but we also want to test how the explicit local-exec works, no? Would we use a macro to and run this entire test with (localexec) and with the empty string to check they produce the same results?

[abrown]

llvm/test/CodeGen/WebAssembly/tls-general-dynamic.ll

2

Yeah, that's what the two lines removed at the top do here. (You'll notice line 5 doesn't target Emscripten but that is just doing a check when bulk memory is disabled, -bulk-memory). I agree that adding a bunch to this file doesn't make much sense. Perhaps I can add a one-off file that targets non-Emscripten and the non-local DSO case and shows how actually in that case we are overriding the codegen to look like localexec. What do you think of something like that, e.g., tls-general-dynamic-override.ll?

Another option is to modify the file as I have here and that's it--nothing extra. I guess the "override with localexec" isn't getting tested specifically then, but it seems like tls-local-exec.ll is covering some of this.

llvm/test/CodeGen/WebAssembly/tls-local-exec.ll

101

Sure, that makes sense. Can you point me to an example of how to setup such a macro?

[sbc100]

llvm/test/CodeGen/WebAssembly/tls-local-exec.ll

101

You can do git grep "FileCheck.* -D" to find a lot of examples in llvm.

[abrown]

Update to fix tests, clang-format

This change used git clang-format origin/main -- llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp to properly format that code and adds some sed substitution in tls-local-exec.ll to check both the "specify localexec explicitly" and "emit localexec even under the generaldynamic default" cases. I believe this should also cover the cases removed from tls-general-dynamic.ll.

[sbc100]

llvm/test/CodeGen/WebAssembly/tls-general-dynamic.ll

6

Should we change the triple here to be wasm32-unknown-emscripten and remove it from the lines above.

Also, might be worth adding a comment above. Something like:

"This is a test for the experimental emscripten-specific general dynamic TLS support. See tls-local-exec.ll for non-emscripten targets (since they lower all TLS to localexec)"

llvm/test/CodeGen/WebAssembly/tls-local-exec.ll

5

Maybe add something like "...on non-emscripten targets which don't currently support dynamic linking."

Also, should we run this with explicit general dynamic too?

[abrown]

Update tests with @sbc100's suggestions

[sbc100]

llvm/test/CodeGen/WebAssembly/tls-local-exec.ll

5

How about explicit general dynamic TLS mode here too (since I think that is what clang generates)?

[abrown]

llvm/test/CodeGen/WebAssembly/tls-local-exec.ll

5

Yeah, here's what I get when I inject (generaldynamic):

/.../llvm-project/build/bin/llc: error: /.../llvm-project/build/bin/llc: <stdin>:103:30: error: expected localdynamic, initialexec or localexec
@tls = internal thread_local(generaldynamic) global i32 0

[sbc100]

lgtm!

[penzn]

This comment was removed by penzn.

llvm/test/CodeGen/WebAssembly/tls-local-exec.ll

101

Shouldn't we leave this file as is, with the explicit TLS model? (according the language reference the default thread model is general dynamic: https://llvm.org/docs/LangRef.html#thread-local-storage-models "If no explicit model is given, the “general dynamic” model is used.").

I guess you are trying to test here that general dynamic is treated "as if" its local-exec, but we also want to test how the explicit local-exec works, no? Would we use a macro to and run this entire test with (localexec) and with the empty string to check they produce the same results?

101

Sure, that makes sense. Can you point me to an example of how to setup such a macro?

101

Shouldn't we leave this file as is, with the explicit TLS model? (according the language reference the default thread model is general dynamic: https://llvm.org/docs/LangRef.html#thread-local-storage-models "If no explicit model is given, the “general dynamic” model is used.").

Target is allowed to change model even if it is supported, "The target may choose a different TLS model if the specified model is not supported, or if a better choice of model can be made."

I guess you are trying to test here that general dynamic is treated "as if" its local-exec, but we also want to test how the explicit local-exec works, no? Would we use a macro to and run this entire test with (localexec) and with the empty string to check they produce the same results?

This might affect more than general-dynamic vs local-exec, but I am not sure it is realistically to test for all possible ones. I think macro would be on the IR side, not FileCheck side (latter should get the same input), and I can't recall how we express something like that.

[penzn]

Oops, commented on outdated version (also had some issues with UI leaving repeated inline comments, sorry).

Out of curiosity, why disabling bulk memory disables TLS? Couldn't find why that is the case in tools conventions.

llvm/test/CodeGen/WebAssembly/tls-local-exec.ll

5

I don't think it is allowed to be specified directly, it is default-only mode.

[sbc100]

In D130053#3675183, @penzn wrote:

Out of curiosity, why disabling bulk memory disables TLS? Couldn't find why that is the case in tools conventions.

Because TLS data is stored a "passive" segment which has to be loaded using the memory.init instruction. (Both passive segments and this instruction are part of the bulk-memory proposal).

Here is the memory.init instruction being output by the linker: https://github.com/llvm/llvm-project/blob/07aa8fc8db6b4b8581e0ba8ef4a66274023c0b59/lld/wasm/Writer.cpp#L1439

[penzn]

Because TLS data is stored a "passive" segment which has to be loaded using the memory.init instruction. (Both passive segments and this instruction are part of the bulk-memory proposal).

Thanks! Should something about this added to tool conventions? I understand why may be it wasn't - this is internal to how compiler can implement TLS, not "handshake" between tools.

[sbc100]

In D130053#3677260, @penzn wrote:

Because TLS data is stored a "passive" segment which has to be loaded using the memory.init instruction. (Both passive segments and this instruction are part of the bulk-memory proposal).

Thanks! Should something about this added to tool conventions? I understand why may be it wasn't - this is internal to how compiler can implement TLS, not "handshake" between tools.

I think this should be tooling-conventions somewhere yes.. I don't think we have anything there yet about TLS or threading.

llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp

LowerCallResults(MachineInstr &CallResults, DebugLoc DL, MachineBasicBlock *BB,
   }
 
   // Move the function pointer to the end of the arguments for indirect calls
   if (IsIndirect) {
     auto FnPtr = CallParams.getOperand(0);
     CallParams.removeOperand(0);
 
     // For funcrefs, call_indirect is done through __funcref_call_table and the
-    // funcref is always installed in slot 0 of the table, therefore instead of having
-    // the function pointer added at the end of the params list, a zero (the index in
+    // funcref is always installed in slot 0 of the table, therefore instead of
+    // having the function pointer added at the end of the params list, a zero
+    // (the index in
     // __funcref_call_table is added).
     if (IsFuncrefCall) {
       Register RegZero =
           MF.getRegInfo().createVirtualRegister(&WebAssembly::I32RegClass);
       MachineInstrBuilder MIBC0 =
           BuildMI(MF, DL, TII.get(WebAssembly::CONST_I32), RegZero).addImm(0);
 
       BB->insert(CallResults.getIterator(), MIBC0);
WebAssemblyTargetLowering::LowerCall(CallLoweringInfo &CLI,
   } else if (IsVarArg) {
     FINode = DAG.getIntPtrConstant(0, DL);
   }
 
   if (Callee->getOpcode() == ISD::GlobalAddress) {
     // If the callee is a GlobalAddress node (quite common, every direct call
     // is) turn it into a TargetGlobalAddress node so that LowerGlobalAddress
     // doesn't at MO_GOT which is not needed for direct calls.
-    GlobalAddressSDNode* GA = cast<GlobalAddressSDNode>(Callee);
+    GlobalAddressSDNode *GA = cast<GlobalAddressSDNode>(Callee);
     Callee = DAG.getTargetGlobalAddress(GA->getGlobal(), DL,
                                         getPointerTy(DAG.getDataLayout()),
                                         GA->getOffset());
     Callee = DAG.getNode(WebAssemblyISD::Wrapper, DL,
                          getPointerTy(DAG.getDataLayout()), Callee);
   }
 
   // Compute the operands for the CALLn node.
WebAssemblyTargetLowering::LowerGlobalTLSAddress(SDValue Op,
 
   MachineFunction &MF = DAG.getMachineFunction();
   if (!MF.getSubtarget<WebAssemblySubtarget>().hasBulkMemory())
     report_fatal_error("cannot use thread-local storage without bulk memory",
                        false);
 
   const GlobalValue *GV = GA->getGlobal();
 
-  // Currently Emscripten does not support dynamic linking with threads.
-  // Therefore, if we have thread-local storage, only the local-exec model
-  // is possible.
-  // TODO: remove this and implement proper TLS models once Emscripten
-  // supports dynamic linking with threads.
-  if (GV->getThreadLocalMode() != GlobalValue::LocalExecTLSModel &&
-      !Subtarget->getTargetTriple().isOSEmscripten()) {
-    report_fatal_error("only -ftls-model=local-exec is supported for now on "
-                       "non-Emscripten OSes: variable " +
-                           GV->getName(),
-                       false);
-  }
-
-  auto model = GV->getThreadLocalMode();
+  // Currently only Emscripten supports dynamic linking with threads. Therefore,
+  // on other targets, if we have thread-local storage, only the local-exec
+  // model is possible.
+  auto model = Subtarget->getTargetTriple().isOSEmscripten()
+                   ? GV->getThreadLocalMode()
+                   : GlobalValue::LocalExecTLSModel;
 
   // Unsupported TLS modes
   assert(model != GlobalValue::NotThreadLocal);
   assert(model != GlobalValue::InitialExecTLSModel);
 
   if (model == GlobalValue::LocalExecTLSModel ||
       model == GlobalValue::LocalDynamicTLSModel ||
       (model == GlobalValue::GeneralDynamicTLSModel &&
if (isPositionIndependent()) {
     const GlobalValue *GV = GA->getGlobal();
     if (getTargetMachine().shouldAssumeDSOLocal(*GV->getParent(), GV)) {
       MachineFunction &MF = DAG.getMachineFunction();
       MVT PtrVT = getPointerTy(MF.getDataLayout());
       const char *BaseName;
       if (GV->getValueType()->isFunctionTy()) {
         BaseName = MF.createExternalSymbolName("__table_base");
         OperandFlags = WebAssemblyII::MO_TABLE_BASE_REL;
-      }
-      else {
+      } else {
         BaseName = MF.createExternalSymbolName("__memory_base");
         OperandFlags = WebAssemblyII::MO_MEMORY_BASE_REL;
       }
       SDValue BaseAddr =
           DAG.getNode(WebAssemblyISD::Wrapper, DL, PtrVT,
                       DAG.getTargetExternalSymbol(BaseName, PtrVT));
 
       SDValue SymAddr = DAG.getNode(

llvm/test/CodeGen/WebAssembly/tls-general-dynamic.ll

-; RUN: not llc < %s -asm-verbose=false -disable-wasm-fallthrough-return-opt -wasm-disable-explicit-locals -mattr=+bulk-memory,atomics 2>&1 | FileCheck %s --check-prefix=ERROR
-; RUN: not llc < %s -asm-verbose=false -disable-wasm-fallthrough-return-opt -wasm-disable-explicit-locals -mattr=+bulk-memory,atomics -fast-isel 2>&1 | FileCheck %s --check-prefix=ERROR
-; RUN: llc < %s -asm-verbose=false -disable-wasm-fallthrough-return-opt -wasm-disable-explicit-locals -mattr=+bulk-memory,atomics --mtriple wasm32-unknown-emscripten | FileCheck %s --check-prefixes=CHECK,TLS
-; RUN: llc < %s -asm-verbose=false -disable-wasm-fallthrough-return-opt -wasm-disable-explicit-locals -mattr=+bulk-memory,atomics --mtriple wasm32-unknown-emscripten -fast-isel | FileCheck %s --check-prefixes=CHECK,TLS
+; This test checks experimental Emscripten-specific `generaldynamic` TLS support. See `tls-local-exec.ll` for non-Emscripten targets (since it lowers all TLS to `localexec`).
+; RUN: llc < %s -asm-verbose=false -disable-wasm-fallthrough-return-opt -wasm-disable-explicit-locals -mattr=+bulk-memory,atomics | FileCheck %s --check-prefixes=CHECK,TLS
+; RUN: llc < %s -asm-verbose=false -disable-wasm-fallthrough-return-opt -wasm-disable-explicit-locals -mattr=+bulk-memory,atomics -fast-isel | FileCheck %s --check-prefixes=CHECK,TLS
 ; RUN: llc < %s -asm-verbose=false -disable-wasm-fallthrough-return-opt -wasm-disable-explicit-locals -mattr=-bulk-memory,atomics | FileCheck %s --check-prefixes=CHECK,NO-TLS
-target triple = "wasm32-unknown-unknown"
-
-; ERROR: LLVM ERROR: only -ftls-model=local-exec is supported for now on non-Emscripten OSes: variable tls
+target triple = "wasm32-unknown-emscripten"
 
 ; CHECK-LABEL: address_of_tls:
 ; CHECK-NEXT: .functype  address_of_tls () -> (i32)
 define i32 @address_of_tls() {
   ; TLS-DAG: global.get __tls_base
   ; TLS-DAG: i32.const tls@TLSREL
   ; TLS-NEXT: i32.add
   ; TLS-NEXT: return

llvm/test/CodeGen/WebAssembly/tls-local-exec.ll

-; RUN: llc < %s -asm-verbose=false -disable-wasm-fallthrough-return-opt -wasm-disable-explicit-locals -mattr=+bulk-memory,atomics | FileCheck %s --check-prefixes=CHECK,TLS
-; RUN: llc < %s -asm-verbose=false -disable-wasm-fallthrough-return-opt -wasm-disable-explicit-locals -mattr=+bulk-memory,atomics -fast-isel | FileCheck %s --check-prefixes=CHECK,TLS
-; RUN: llc < %s -asm-verbose=false -disable-wasm-fallthrough-return-opt -wasm-disable-explicit-locals -mattr=-bulk-memory,atomics | FileCheck %s --check-prefixes=CHECK,NO-TLS
+; Run the tests with the `localexec` TLS mode specified.
+; RUN: sed -e 's/\[\[TLS_MODE\]\]/(localexec)/' %s | llc -asm-verbose=false -disable-wasm-fallthrough-return-opt -wasm-disable-explicit-locals -mattr=+bulk-memory,atomics - | FileCheck --check-prefixes=CHECK,TLS %s
+; RUN: sed -e 's/\[\[TLS_MODE\]\]/(localexec)/' %s | llc -asm-verbose=false -disable-wasm-fallthrough-return-opt -wasm-disable-explicit-locals -mattr=+bulk-memory,atomics -fast-isel - | FileCheck --check-prefixes=CHECK,TLS %s
+
+; Also, run the same tests without a specified TLS mode--this should still emit `localexec` code on non-Emscripten targtes which don't currently support dynamic linking.
+; RUN: sed -e 's/\[\[TLS_MODE\]\]//' %s | llc -asm-verbose=false -disable-wasm-fallthrough-return-opt -wasm-disable-explicit-locals -mattr=+bulk-memory,atomics - | FileCheck --check-prefixes=CHECK,TLS %s
+; RUN: sed -e 's/\[\[TLS_MODE\]\]//' %s | llc -asm-verbose=false -disable-wasm-fallthrough-return-opt -wasm-disable-explicit-locals -mattr=+bulk-memory,atomics -fast-isel - | FileCheck --check-prefixes=CHECK,TLS %s
+
+; Finally, when bulk memory is disabled, no TLS code should be generated.
+; RUN: sed -e 's/\[\[TLS_MODE\]\]/(localexec)/' %s | llc -asm-verbose=false -disable-wasm-fallthrough-return-opt -wasm-disable-explicit-locals -mattr=-bulk-memory,atomics - | FileCheck --check-prefixes=CHECK,NO-TLS %s
 target triple = "wasm32-unknown-unknown"
 
 ; CHECK-LABEL: address_of_tls:
 ; CHECK-NEXT: .functype  address_of_tls () -> (i32)
 define i32 @address_of_tls() {
   ; TLS-DAG: global.get __tls_base
   ; TLS-DAG: i32.const tls@TLSREL
   ; TLS-NEXT: i32.add

 }
 
 ; CHECK: .type tls,@object
 ; TLS-NEXT: .section .tbss.tls,"T",@
 ; NO-TLS-NEXT: .section .bss.tls,"",@
 ; CHECK-NEXT: .p2align 2
 ; CHECK-NEXT: tls:
 ; CHECK-NEXT: .int32 0
-@tls = internal thread_local(localexec) global i32 0
+@tls = internal thread_local[[TLS_MODE]] global i32 0
 
-@tls_external = external thread_local(localexec) global i32, align 4
+@tls_external = external thread_local[[TLS_MODE]] global i32, align 4
 
 declare i32 @llvm.wasm.tls.size.i32()