[PATCH] D101608: [WebAssembly] Support for WebAssembly globals in LLVM IR

[Sam Clegg via Phabricator via cfe-commits]

sbc100 added inline comments.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1279
+  if (const GlobalAddressSDNode *GA = dyn_cast<GlobalAddressSDNode>(Op))
+    return WebAssembly::isManagedAddressSpace(GA->getAddressSpace());
+
----------------
sunfish wrote:
> tlively wrote:
> > tlively wrote:
> > > Actually, should we enforce that these LLVM IR globals be thread_local, since the resulting Wasm globals will be thread_local? I don't know if that will affect any optimizations, but it seems like a more accurate modeling. If we do that, `CoalesceLocalsAndStripAtomics::stripThreadLocals` in WebAssemblyTargetMachine.cpp will have to be updated to not strip the thread locals corresponding to Wasm globals.
> > I'd be fine handling this in a follow-up if you want to get this landed.
> Note that this will be true of Worker-based threads, but not of the expected future "wasm native threads". I wonder if this means that clang/llvm will (eventually) need to be aware of this difference.
Don't you think is very likely that even in "wasm native threads" we would want to opt into sharing like we do with wasm memories?   That path would seem better for compatibility with existing worker-based threading.  

Obviously once we do have shared wasm globals we will need to modify llvm's assumptions, but for now I think it is a reasonable assumption/assertion that wasm globals are TLS.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101608