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

[Heejin Ahn via Phabricator via llvm-commits]

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


================
Comment at: lld/test/wasm/tls.ll:6
+ at tls1 = thread_local(localexec) global i32 1, align 4
+ at tls2 = thread_local(localexec) global i32 1, align 4
+
----------------
Can we possibly mix one non-tls global variable between `tls1` and `tls2`, just to see they work together?


================
Comment at: lld/wasm/Writer.cpp:629
+  // Merge .tbss into .tdata so that they share the same offsets.
+  if (name.startswith(".tbss."))
+    return ".tdata";
----------------
quantum wrote:
> sbc100 wrote:
> > Maybe write this as a single conditional so its clear even without this comment?
> Changed in latest diff.
Has it been reflected? It says it has been changed but it doesn't look like it has


================
Comment at: lld/wasm/Writer.cpp:431
+    error("'bulk-memory' feature must be used in order to use thread-local "
+          "storage");
+
----------------
quantum wrote:
> quantum wrote:
> > aheejin wrote:
> > > Should we check for "mutable-global" feature too?
> > Do we need to? I thought it's always available since we use it for the stack pointer.
> On second thought, the `mutable-global` feature is for import/export of mutable globals. TLS does not need to do this.
Ah you're right.


================
Comment at: lld/wasm/Writer.cpp:514
       if (G->getGlobalType()->Mutable) {
         // Only the __stack_pointer should ever be create as mutable.
+        assert(G == WasmSym::StackPointer || G == WasmSym::TLSBase);
----------------
quantum wrote:
> aheejin wrote:
> > Now `__tls_base` is also mutable, I think we should fix the comment
> Will do.
PIng. It doesn't look like fixed yet


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelDAGToDAG.cpp:203
+    ReplaceUses(SDValue(Node, 0), SDValue(TLSAddress, 0));
+    CurDAG->RemoveDeadNode(Node);
+    return;
----------------
These two lines can be shortened to
```
ReplaceNode(Node, TLSAddress);
```
The same applies to the code below for `__tls_size` too.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelDAGToDAG.cpp:208
+  case ISD::INTRINSIC_WO_CHAIN:
+    switch (cast<ConstantSDNode>(Node->getOperand(0))->getZExtValue()) {
+    case Intrinsic::wasm_tls_size: {
----------------
Nit: Might be a bit cleaner to extract the expression, which is complicated, like
```
unsigned IntNo = cast<ConstantSDNode>(Node->getOperand(0))->getZExtValue());
switch (IntNo) {
 ...
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64537