[PATCH] D90948: [WebAssembly] [WIP] call_indirect issues table number relocs

[Andy Wingo via Phabricator via llvm-commits]

wingo added a comment.

So, just to recap my understanding: before this patch, LLVM will indeed generate the `__indirect_function_table` import and add a corresponding entry to the "linking" section.  However it won't emit relocs in `call_indirect` instructions, and for the table number operand, LLVM just writes a single zero byte instead of a relocatable 5-byte LEB.

With this patch *and the reference types feature enabled*, what we get is the same thing, except with relocs and 5-byte relocatable call_indirect.

Given that `call_indirect` is much less frequent than other relocs (?), I think it might be reasonable to reduce the configuration-space here and just lower to `CALL_INDIRECT_TABLE` even when the reference-types feature is not enabled.  WDYT @sbc100 ?  We would then only emit the `__indirect_function_table import` if the object contains a `call_indirect`.  For all existing code this should continue to result in there being a single table, numbered 0, so the result would be compatible with the MVP.  We can find a way to continue to allow `call_indirect` to parse without an explicit table argument.

See the similar discussion over at https://github.com/WebAssembly/wabt/pull/1549.



================
Comment at: llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:611
+              WasmSym->setType(SymbolType.getValue());
+            }
+          }
----------------
sbc100 wrote:
> I guess it makes sense to piggy back on the existing symbol handling at the end of the instruction...  but would having the symbol immediate come before the signature make more sense for this instruction?
I wasn't sure, tbh, and in the end went with the order in which the fields appear in the binary: https://webassembly.github.io/reference-types/core/binary/instructions.html#control-instructions.  If you think I should change it, happy to do so.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90948