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

[Thomas Lively via Phabricator via llvm-commits]

tlively added inline comments.


================
Comment at: llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:611
+              WasmSym->setType(SymbolType.getValue());
+            }
+          }
----------------
wingo wrote:
> wingo wrote:
> > wingo wrote:
> > > tlively wrote:
> > > > wingo wrote:
> > > > > 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.
> > > > In the text format the table index appears first. I think it would be good to be consistent with that rather than the binary format.
> > > Sure.
> > Just to confirm, we want the text format to have a different operand order than the CALL_INDIRECT instruction from WebAssemblyCall.td -- can do.
> Hummm, I will have to write a dedicated printer in WebAssemblyInstrPrint.cpp for `{RET_,}CALL_INDIRECT_S`, to invert the operand order.  Really sorry to bother but @tlively can you confirm this is the right thing?  Thanks :)
Yes, confirmed. I hadn't thought about how this will need a custom printer, but I still think it is the right thing to do. Folks who read and write this assembly format will probably also be spending time at least reading standard wat/wast files and probably not much time looking at the raw binaries, so I think it would be most helpful to the user for us to match the text format rather than the binary format here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90948