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

[Sam Clegg via Phabricator via llvm-commits]

sbc100 accepted this revision.
sbc100 added a comment.
This revision is now accepted and ready to land.

This looks great!



================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:179
+
+  assert(!GV->isThreadLocal());
+
----------------
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).


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:204
+    OutStreamer->emitLabel(Sym);
+    OutStreamer->AddBlankLine();
+  }
----------------
There is no actual initializer here.. it will always be zero I guess?  Do we need a TODO?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:216
+      if (Sym->isUndefined())
+        getTargetStreamer()->emitGlobalType(Sym);
+    } else if (Sym->getType() == wasm::WASM_SYMBOL_TYPE_EVENT)
----------------
Why not leave this code alone and omit the `emitGlobalType` above?


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


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp:68
+      WasmSym->setType(wasm::WASM_SYMBOL_TYPE_GLOBAL);
+      WasmSym->setGlobalType(wasm::WasmGlobalType{uint8_t(Type), Mutable});
+    }
----------------
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 ? :-/


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101913