[PATCH] D64537: [WebAssembly] Implement thread-local storage (local-exec model)

[Thomas Lively via Phabricator via cfe-commits]

tlively added inline comments.


================
Comment at: lld/wasm/Driver.cpp:543
+      "__wasm_init_tls", WASM_SYMBOL_VISIBILITY_HIDDEN,
+      make<SyntheticFunction>(I32ArgSignature, "__wasm_init_tls"));
+
----------------
quantum wrote:
> aheejin wrote:
> > Does this TLS thing work when `Config->Shared == true`, i.e., do we create TLS globals even when this is a library?
> Since TLS is module specific (we can't allocate memory for other modules), we must in fact generate this symbol for every module. Shared library support will not be implemented in this diff, however.
Until we do implement TLS for shared modules, would it make sense to omit these globals and function from shared modules, since we can't use them in that context anyway?


================
Comment at: lld/wasm/Writer.cpp:638
+  if (Name.startswith(".tbss."))
+    return ".tdata";
   return Name;
----------------
quantum wrote:
> tlively wrote:
> > Does this mean we can't control whether .tdata or .tbss comes first? Is that important for anything?
> Yes, it does mean that. The only reason why .tbss is supposed to be separate is so that its memory can just be zeroed whereas .tdata has to have the bytes stored in the program image. Currently, we just explicitly store the zero bytes, so this won't be a problem.
It would be really great if we could find a way to elide the .bss 0 bytes as a code size optimization. Since we can't assume that the memory is already zeroed the way we can with passive segments, perhaps we can use a memory.fill instruction to zero the memory? Pursuing this in a follow-on CL should be fine.


================
Comment at: lld/wasm/Writer.cpp:805
+      writeUleb128(os, 0, "num locals");
+      writeU8(os, WASM_OPCODE_END, "end function");
+    }
----------------
You could avoid duplicating these lines by making them unconditional.


================
Comment at: lld/wasm/Writer.cpp:905
+  if (config->sharedMemory)
+    createInitTLSFunction();
+
----------------
Can you remind me how the InitTLSFunction interacts with relocatable code? I'm wondering if this should be called up in the condition with the other synthetic functions.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp:193
+
+    if (!Features[WebAssembly::FeatureBulkMemory])
       Stripped |= stripThreadLocals(M);
----------------
I just realized that if we have atomics but not bulk memory and TLS is stripped, then we will be in the awkward situation of both using atomics and disallowing atomics because the module is not thread safe. I think the best solution would be to go back and forcibly strip both atomics and TLS if either of them would be stripped.


================
Comment at: llvm/test/CodeGen/WebAssembly/tls.ll:2
+; RUN: llc < %s -asm-verbose=false -disable-wasm-fallthrough-return-opt -wasm-disable-explicit-locals -mattr=+bulk-memory | FileCheck %s
+; RUN: llc < %s -asm-verbose=false -disable-wasm-fallthrough-return-opt -wasm-disable-explicit-locals -mattr=+bulk-memory -fast-isel | FileCheck %s
 target datalayout = "e-m:e-p:32:32-i64:64-n32:64-S128"
----------------
It would be good to check the negative case, too, i.e with bulk-memory disabled.


================
Comment at: llvm/test/CodeGen/WebAssembly/tls.ll:6
 
-; SINGLE-LABEL: address_of_tls:
+; CHECK-LABEL: address_of_tls:
 define i32 @address_of_tls() {
----------------
Is `CHECK` still used as a prefix if it not listed in the invocation of FileCheck?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64537