[PATCH] D91637: [WebAssembly] Only emit indirect function table import if needed

Tue Nov 24 07:31:58 PST 2020

wingo added inline comments.

================
Comment at: llvm/lib/MC/WasmObjectWriter.cpp:503
+      Type == wasm::R_WASM_TABLE_INDEX_I64) {
+    // TABLE_INDEX relocs implicitly use the default indirect function table.
+    auto TableName = "__indirect_function_table";
----------------
sbc100 wrote:
> Perhaps add a TODO here since we probably don't to keep this here long right? 
We keep this one, I think.  Currently `call_indirect` has no relocs.  In the future it will have `TABLE_NUMBER` relocs.  `TABLE_INDEX` relocs will stay around, used for function pointers.

In some future it might be useful to have the `TABLE_INDEX` relocs specify an "address space" in the form of a table number (table symbol reference); i.e. the `EXTENDED_TABLE_INDEX` reloc would be against both a function symbol and a table symbol.  But until then, and until such a use case arises, I think `TABLE_INDEX` relocs still implicitly use `__indirect_function_table`.  WDYT?

================
Comment at: llvm/test/MC/WebAssembly/reloc-pic.s:94
+# CHECK-NEXT:           Value:           1
+# CHECK-NEXT:         Functions:       [ 5 ]
 # CHECK-NEXT:   - Type:            DATACOUNT
----------------
sbc100 wrote:
> What happened here?
This is the result of replacing the whole `CHECK`+`CHECK_NEXT`* block with the output of the command in question.  Harmless AFAIU, but extraneous also, so I have removed it.

================
Comment at: llvm/test/MC/WebAssembly/tables.s:211
+# BIN-NEXT:        Table:           3
+# BIN-NEXT:      - Index:           4
----------------
sbc100 wrote:
> Where these here before but just not being checked for I guess?
I just checked and yes, they were here before and not being checked for.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91637