[PATCH] D119803: [WebAssembly] Make __wasm_lpad_context thread-local
Sam Clegg via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Feb 15 08:28:02 PST 2022
sbc100 added inline comments.
================
Comment at: llvm/lib/CodeGen/WasmEHPrepare.cpp:220
+ // the side effect of disallowing the object from being linked into a
+ // shared-memory module, which we don't want to be responsible for.
LPadContextGV = cast<GlobalVariable>(
----------------
If this object file was built without atomics and then this code didn't mark the variable as TLS then isn't this object incompatible with shared-memory at link time? In fact wouldn't it fail to link with the defintion of `__wasm_lpad_context` which is going to be TLS?
i.e. object files build with exception handling (i.e. references to __wasm_lpad_context) but not +atomics are not compatible with shared memory at link time, right?
================
Comment at: llvm/lib/CodeGen/WasmEHPrepare.cpp:227
+ StringRef FS = FSAttr.getValueAsString();
+ if (FS.contains("+atomics") && FS.contains("+bulk-memory"))
+ EnableTLS = true;
----------------
For simplicity can we just check for atomics?
================
Comment at: llvm/lib/CodeGen/WasmEHPrepare.cpp:232
+ : GlobalValue::NotThreadLocal;
+ LPadContextGV->setThreadLocalMode(TLS);
+
----------------
Can we simplify the code and avoid the `TLS` and `EnableTLS` variables and just do this above?
```
if (FS.contains("+atomics") && FS.contains("+bulk-memory"))
LPadContextGV->setThreadLocalMode(GlobalValue::GeneralDynamicTLSModel)
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D119803/new/
https://reviews.llvm.org/D119803
More information about the llvm-commits
mailing list