[PATCH] D119803: [WebAssembly] Make __wasm_lpad_context thread-local

[Sam Clegg via Phabricator via llvm-commits]

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