[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/WebAssemblyMCInstLower.cpp:175
+      WasmSym->setTableType(TableType);
+    }
+
----------------
pmatos wrote:
> 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.
Me: Who was the fool who set the type of a table `GetGlobalAddressSymbol` to `WASM_SYMBOL_TYPE_GLOBAL`?

`git blame`: That was you...

Me: Doh!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115511