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

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added a subscriber: tlively.
aheejin 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>(
----------------
sbc100 wrote:
> 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?
I don't really remember details about that. I mostly copy-pasted this: https://github.com/llvm/llvm-project/blob/6822d89e776960caa753fd4181fe99feaef5032b/llvm/lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp#L418-L427
@tlively may know the details about the rules..?

The difference is the existing code checks the subtarget to check the features, but here in lib/CodeGen we don't have the access to `TargetMachine` so I checked function attributes.


================
Comment at: llvm/lib/CodeGen/WasmEHPrepare.cpp:227
+    StringRef FS = FSAttr.getValueAsString();
+    if (FS.contains("+atomics") && FS.contains("+bulk-memory"))
+      EnableTLS = true;
----------------
sbc100 wrote:
> For simplicity can we just check for atomics?
Again, I just followed https://github.com/llvm/llvm-project/blob/6822d89e776960caa753fd4181fe99feaef5032b/llvm/lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp#L418-L427.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119803/new/

https://reviews.llvm.org/D119803