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

[Sam Clegg via Phabricator via llvm-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:
> sbc100 wrote:
> > 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.
> Yeah, I agree we'll likely want some way to opt into sharing. So the difference here is, worker-based threads won't have the option of sharing, so maybe requiring `thread_local` makes sense. But native threads could opt into sharing, so they could be `thread_local` or not.
> 
Yup.  And who know .. maybe by the time shared globals and native threads are added, Workers might also add shared globals?  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101608