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

Guanzhong Chen via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Jul 15 13:59:00 PDT 2019


quantum added inline comments.


================
Comment at: lld/test/wasm/data-segments.ll:7
 ; RUN: wasm-ld -no-gc-sections --no-entry --shared-memory --max-memory=131072 %t.atomics.o -o %t.atomics.wasm
-; RUN: obj2yaml %t.atomics.wasm | FileCheck %s --check-prefix ACTIVE
+; RUN: obj2yaml %t.atomics.wasm | FileCheck %s --check-prefixes ACTIVE,ACTIVE-TLS
 
----------------
aheejin wrote:
> quantum wrote:
> > aheejin wrote:
> > > What is the difference between `ACTIVE` and `ACTIVE-TLS`? It looks we don't have different build processes for them. And as what @sbc100 said, can we exclude TLS from build?
> > `ACTIVE-TLS` is for builds with TLS enabled. Currently, we use `--shared-memory` to determine that, per @tlively's recommendation. The rationale is that we don't want even more flags that need to be passed in a proper threaded build.
> Then if we don't enable `--shared-memory`, we don't generate those globals? Do we have a test for that?
Yes. In this file, the cases with `--check-prefix ACTIVE` will ensure that the fields are not generated.


================
Comment at: lld/wasm/Driver.cpp:543
+      "__wasm_init_tls", WASM_SYMBOL_VISIBILITY_HIDDEN,
+      make<SyntheticFunction>(I32ArgSignature, "__wasm_init_tls"));
+
----------------
tlively wrote:
> 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?
That would make sense for now.


================
Comment at: lld/wasm/Writer.cpp:638
+  if (Name.startswith(".tbss."))
+    return ".tdata";
   return Name;
----------------
tlively wrote:
> 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.
Yes, this is a good idea for a follow-on CL.


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


================
Comment at: lld/wasm/Writer.cpp:905
+  if (config->sharedMemory)
+    createInitTLSFunction();
+
----------------
tlively wrote:
> 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.
I don't think it should. `__wasm_init_tls` initializes the TLS block for the current module only, so every shared library needs to have its own `__wasm_init_tls`.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp:193
+
+    if (!Features[WebAssembly::FeatureBulkMemory])
       Stripped |= stripThreadLocals(M);
----------------
tlively wrote:
> 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.
Done.


================
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"
----------------
tlively wrote:
> It would be good to check the negative case, too, i.e with bulk-memory disabled.
Done.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64537





More information about the llvm-commits mailing list