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

[Andy Wingo via Phabricator via cfe-commits]

wingo 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
----------------
sunfish wrote:
> tlively wrote:
> > sunfish wrote:
> > > 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.
> > > 
> > > 
> > Sure, that works for me. We may want to rename it in the future if we end up using the same address space for tables and memories, but we can cross that bridge when we get there.
> Tables and memories have their own index spaces in wasm, so it seems like they'd want their own address spaces here, perhaps 2 and 3, respectively? I also agree that we can figure this out later.
`WASM_VARS` is fine with me, with one caveat -- in https://reviews.llvm.org/D101140  where we add support for locals, we add a new "core" TargetStackID enum which describes static `alloca` that is actually a wasm local.  Really this enum describes objects allocated on a parallel stack, outside of linear memory.  It was nice to use the same name to describe the address space and the stack id, but I don't think `variable` or `wasm-var` are good core enum names.  It feels like there is a core concept here for systems that have two stacks -- one of named objects and one of linear-memory bytes.  I thought `object` or `managed` could be a good name for the former, but evidently not :)

I will let this one sit overnight; if there are no other good ideas before tomorrow I will do `WASM_ADDRESS_SPACE_WASM_VARS` and either find a new name for `TargetStackID` or attempt to use a target-specific definition.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101608