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

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added inline comments.


================
Comment at: llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCTargetDesc.h:118
+  // The symbol declares a funcref table
+  MO_SYM_IS_FUNCREF_TABLE,
 };
----------------
I'm not sure this enumeration is best place to declare that type of a symbols.  These other values all refer to how the symbol is used, but what type the symbol is.   For example that distinction between function and data symbols does not appear here.



================
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);
----------------
Why MO_TABLE_BASE_REL here?   


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp:175
+      WasmSym->setTableType(TableType);
+    }
+
----------------
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`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115511