[PATCH] D101913: [CodeGen][WebAssembly] Better lowering for WASM_SYMBOL_TYPE_GLOBAL symbols
Sam Clegg via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed May 5 16:01:08 PDT 2021
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
More information about the llvm-commits
mailing list