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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
