[PATCH] D101913: [CodeGen][WebAssembly] Better lowering for WASM_SYMBOL_TYPE_GLOBAL symbols

[Andy Wingo via Phabricator via llvm-commits]

wingo added inline comments.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:179
+
+  assert(!GV->isThreadLocal());
+
----------------
sbc100 wrote:
> Why this?   As it happens I think that globals are *all* thread local by default (at least we assume they are in the current threading ABI in llvm and emscripten).   We don't have a way to share global between threads to (like we do with memory).
Good point; I hadn't thought about this.  For context I was looking at what the base "getGlobalVariable" does and where it has conditional blocks for non-default flags, and asserting that no one had made non-default settings on the symbol.

But... semantically what's the story here?  I guess that although you can share globals between instances, you can't post them to a web worker, so they are always thread-local.

Should WASM_SYMBOL_TYPE_GLOBAL symbols be marked by default as being thread-local then, perhaps?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:204
+    OutStreamer->emitLabel(Sym);
+    OutStreamer->AddBlankLine();
+  }
----------------
sbc100 wrote:
> There is no actual initializer here.. it will always be zero I guess?  Do we need a TODO?
Added the TODO.  This one is tricky because there's no MC support for global initializers yet, AFAIU.

Not sure how it should work, tbh; though the binary format specifies that this should be a sequence of constant instructions followed by `end`, I think I would want to restrict this one from the MC side to be some form of MCConstantExpr, serialized either as part of the `.globaltype` line or as a new `.globalvalue SYM, EXPR` form.  WDYT?  Would be a followup I guess.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:216
+      if (Sym->isUndefined())
+        getTargetStreamer()->emitGlobalType(Sym);
+    } else if (Sym->getType() == wasm::WASM_SYMBOL_TYPE_EVENT)
----------------
sbc100 wrote:
> Why not leave this code alone and omit the `emitGlobalType` above?
Because we need to record the type of the extern global somewhere in the .s file.  Will add to comment.  Sound OK to you?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:304
+    if (!G.hasInitializer() && G.hasExternalLinkage() &&
+        !WebAssembly::isObjectAddressSpace(G.getAddressSpace()) &&
+        G.getValueType()->isSized()) {
----------------
sbc100 wrote:
> Similar to other PR.. I wish we could find a better work than `Object` in `isObjectAddressSpace`
Changed to "managed"; lmk what you think!


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp:68
+      WasmSym->setType(wasm::WASM_SYMBOL_TYPE_GLOBAL);
+      WasmSym->setGlobalType(wasm::WasmGlobalType{uint8_t(Type), Mutable});
+    }
----------------
sbc100 wrote:
> Regarding the duplication.. I think we already suffer from a little of that with the existing code for handling functions (See `WasmSym->setSignature` below and in `emitFunctionBodyStart` and in `emitEndOfAsmFile`).   So I guess the makes it ok ? :-/
Fair enough :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101913