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
+@tls1 = thread_local(localexec) global i32 1, align 4
+@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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
