[PATCH] D64758: [WebAssembly] Assembler/InstPrinter: support call_indirect type index.

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added a comment.

I think the problem here is that there is no such thing as a first class "type" symbol, there are only functions symbols that can be referenced with `@TYPE_INDEX` to find where they live in the type index space.

Do we need a first class "type" symbol?   Perhaps we do?   Also can call_indirect take a signature literal in the asm syntax.  i.e. do we want to allow all of the following:

  call_indirect some_function at TYPEINDEX
  call_indirect some_type
  call_indirect (i32) -> (i32)

Right now it seems like we are only supporting the first one, and this change basically declared a face function symbol to hang that type off of.

It seems clear that we *do* want to support populating the type section and referencing those types without declaring any functions at all.    You can imagine a module that does all kind of call_indirect on an imported table without actually declaring any functions at all.  So I think I'm arguing that we a first calls type symbol as well as the some_function at TYPEINDEX syntax.



================
Comment at: lib/Target/WebAssembly/MCTargetDesc/WebAssemblyInstPrinter.cpp:58
+      auto SRE = static_cast<const MCSymbolRefExpr *>(O.getExpr());
+      if (SRE->getKind() == MCSymbolRefExpr::VK_WASM_TYPEINDEX) {
+        auto &Sym = static_cast<const MCSymbolWasm &>(SRE->getSymbol());
----------------
This seems like this should be unnecessary.

The only types of symbols that can be referenced with VK_WASM_TYPEINDEX are function symbols.   i.e. should be able to assert here that `SRE->getSymbol` is a function symbo.l.  And functions symbols should already if their type specified in the assembly when they are declared.



================
Comment at: lib/Target/WebAssembly/MCTargetDesc/WebAssemblyInstPrinter.h:62
+std::string typeListToString(ArrayRef<wasm::ValType> List);
+std::string signatureToString(const wasm::WasmSignature *Sig);
+
----------------
Seems a little odd to have these two "ToString" functions return std::string but the two above return C strings.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64758