[PATCH] D115511: [WebAssembly] Lower global syms representing tables with .tabletype

[Paulo Matos via Phabricator via llvm-commits]

pmatos added inline comments.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1540
+            GA->getGlobal(), DL, GA->getValueType(0), GA->getOffset(),
+            WebAssemblyII::MO_TABLE_BASE_REL));
+    DAG.ReplaceAllUsesWith(GA, NewGA);
----------------
sbc100 wrote:
> Why MO_TABLE_BASE_REL here?   
That's a mistake. It should be `MO_SYM_IS_..._TABLE`.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp:175
+      WasmSym->setTableType(TableType);
+    }
+
----------------
sbc100 wrote:
> If you do want to set the symbol type at this point why do it inside this `if (MO.getOffset() != 0) {` condition?
> 
> However, it seems like for other symbol types the type is already previously set elsewhere so this maybe is not the place to do it?
> 
> Looking at all the places where wasm symbols get their types set it looks like a rather mixed bag:
> 
> ```
> $ git grep 'setType(wasm::'
> llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:        SPSym->setType(wasm::WASM_SYMBOL_TYPE_GLOBAL);
> llvm/lib/MC/MCContext.cpp:  cast<MCSymbolWasm>(Begin)->setType(wasm::WASM_SYMBOL_TYPE_SECTION);
> llvm/lib/MC/MCParser/WasmAsmParser.cpp:      WasmSym->setType(wasm::WASM_SYMBOL_TYPE_FUNCTION);
> llvm/lib/MC/MCParser/WasmAsmParser.cpp:      WasmSym->setType(wasm::WASM_SYMBOL_TYPE_GLOBAL);
> llvm/lib/MC/MCParser/WasmAsmParser.cpp:      WasmSym->setType(wasm::WASM_SYMBOL_TYPE_DATA);
> llvm/lib/MC/MCWasmStreamer.cpp:    Symbol->setType(wasm::WASM_SYMBOL_TYPE_FUNCTION);
> llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:      WasmSym->setType(wasm::WASM_SYMBOL_TYPE_FUNCTION);
> llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:      WasmSym->setType(wasm::WASM_SYMBOL_TYPE_GLOBAL);
> llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:      WasmSym->setType(wasm::WASM_SYMBOL_TYPE_TABLE);
> llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:      WasmSym->setType(wasm::WASM_SYMBOL_TYPE_FUNCTION);
> llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:      WasmSym->setType(wasm::WASM_SYMBOL_TYPE_TAG);
> llvm/lib/Target/WebAssembly/Disassembler/WebAssemblyDisassembler.cpp:        WasmSym->setType(wasm::WASM_SYMBOL_TYPE_FUNCTION);
> llvm/lib/Target/WebAssembly/Utils/WebAssemblyUtilities.cpp:    Sym->setType(wasm::WASM_SYMBOL_TYPE_TABLE);
> llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:    Sym->setType(wasm::WASM_SYMBOL_TYPE_GLOBAL);
> llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:    WasmSym->setType(wasm::WASM_SYMBOL_TYPE_GLOBAL);
> llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:    WasmSym->setType(wasm::WASM_SYMBOL_TYPE_DATA);
> llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:    WasmSym->setType(wasm::WASM_SYMBOL_TYPE_TAG);
> llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:    WasmSym->setType(wasm::WASM_SYMBOL_TYPE_FUNCTION);
> llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:      Sym->setType(wasm::WASM_SYMBOL_TYPE_FUNCTION);
> llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:  WasmSym->setType(wasm::WASM_SYMBOL_TYPE_FUNCTION);
> llvm/lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp:      WasmSym->setType(wasm::WASM_SYMBOL_TYPE_GLOBAL);
> llvm/lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp:  WasmSym->setType(wasm::WASM_SYMBOL_TYPE_FUNCTION);
> llvm/lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp:  WasmSym->setType(wasm::WASM_SYMBOL_TYPE_FUNCTION);
> ```
> 
> Do you have any information to do this t `WebAssemblyMCInstLower::GetGlobalAddressSymbol`?
Yeah - I got the same feeling. Rather confusing about the best place to set this information. I will take a look at `WebAssemblyMCInstLower::GetGlobalAddressSymbol`. Thanks for the suggestion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115511