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:
> 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.
================
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
+
----------------
aheejin wrote:
> Can we possibly mix one non-tls global variable between `tls1` and `tls2`,
> just to see they work together?
Done.
================
Comment at: lld/wasm/Writer.cpp:629
+ // Merge .tbss into .tdata so that they share the same offsets.
+ if (name.startswith(".tbss."))
+ return ".tdata";
----------------
aheejin wrote:
> 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
I think you are commenting on an old version? It is a single condition on the
latest version.
================
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);
----------------
aheejin wrote:
> 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
Looks like the fix got rebasing against the change that made everything
lowerCamelCase. Going to do it again.
================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelDAGToDAG.cpp:203
+ ReplaceUses(SDValue(Node, 0), SDValue(TLSAddress, 0));
+ CurDAG->RemoveDeadNode(Node);
+ return;
----------------
aheejin wrote:
> These two lines can be shortened to
> ```
> ReplaceNode(Node, TLSAddress);
> ```
> The same applies to the code below for `__tls_size` too.
Nice. I didn't know that was possible. Changing.
================
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: {
----------------
aheejin wrote:
> Nit: Might be a bit cleaner to extract the expression, which is complicated,
> like
> ```
> unsigned IntNo = cast<ConstantSDNode>(Node->getOperand(0))->getZExtValue());
> switch (IntNo) {
> ...
> ```
Done.
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
