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

[Dan Gohman via Phabricator via llvm-commits]

sunfish added inline comments.


================
Comment at: llvm/lib/Target/WebAssembly/Utils/WebAssemblyUtilities.h:39
+  // pointers are lowered to global.get / global.set or local.get / local.set,
+  // as appropriate.
+  WASM_ADDRESS_SPACE_MANAGED = 1
----------------
tlively wrote:
> sunfish wrote:
> > Sorry to throw more paint at the bikeshed here, but as someone who's only following along at a high-level here, I found it confusing whether this is talking about the wasm globals themselves, or the objects referred to by reference values in the wasm globals. I think the feature here is talking about the wasm globals themselves, but "managed" initially made me think it might be talking about the objects they reference, which in a browser context especially are "managed" in every sense of the word.
> Fair point. What does everyone think about `INDEXED`, because it is used to represent objects given static indexes (in the WebAssembly sense) in the final binary.
Do I understand correctly that global variables and local variables are being assigned addresses within the same conceptual address space here?

How about `WASM_VARIABLES` or `WASM_VARS`? The wasm spec terms for these are global variables and local variables.




================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1279
+  if (const GlobalAddressSDNode *GA = dyn_cast<GlobalAddressSDNode>(Op))
+    return WebAssembly::isManagedAddressSpace(GA->getAddressSpace());
+
----------------
sbc100 wrote:
> 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?  
It's possible :-). In any case, I don't think it's anything we need to do anything about right now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101608