tlively added inline comments.
================
Comment at: lld/wasm/Driver.cpp:543
+ "__wasm_init_tls", WASM_SYMBOL_VISIBILITY_HIDDEN,
+ make<SyntheticFunction>(I32ArgSignature, "__wasm_init_tls"));
+
----------------
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?
================
Comment at: lld/wasm/Writer.cpp:638
+ if (Name.startswith(".tbss."))
+ return ".tdata";
return Name;
----------------
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.
================
Comment at: lld/wasm/Writer.cpp:805
+ writeUleb128(os, 0, "num locals");
+ writeU8(os, WASM_OPCODE_END, "end function");
+ }
----------------
You could avoid duplicating these lines by making them unconditional.
================
Comment at: lld/wasm/Writer.cpp:905
+ if (config->sharedMemory)
+ createInitTLSFunction();
+
----------------
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.
================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp:193
+
+ if (!Features[WebAssembly::FeatureBulkMemory])
Stripped |= stripThreadLocals(M);
----------------
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.
================
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"
----------------
It would be good to check the negative case, too, i.e with bulk-memory disabled.
================
Comment at: llvm/test/CodeGen/WebAssembly/tls.ll:6
-; SINGLE-LABEL: address_of_tls:
+; CHECK-LABEL: address_of_tls:
define i32 @address_of_tls() {
----------------
Is `CHECK` still used as a prefix if it not listed in the invocation of
FileCheck?
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
