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

[Thomas Lively via Phabricator via llvm-commits]

tlively accepted this revision.
tlively added a comment.

I have two remaining concerns about this patch:

1. I still feel that creating a table symbol that won't be emitted into the binary is an overly complex way of signaling that a table import is needed. I would prefer a simple `shouldImportTable` boolean somewhere to record that information. However, I don't want to hold this patch up any further on that concern because I don't have a good idea of where that boolean should go. I would be happy if we could return to this point once things are working end-to-end.
2. The comment I left on the "magic" table operand insertion this patch adds to the assembler. As I wrote, I would prefer to error out instead of implicitly inserting the missing operand, but that's a relatively minor detail that is unrelated to most of the work done in this patch, so I would happy to leave further discussion of that to a follow-up as well.

I do want to continue discussing those points, but I know this is a largish patch that has been outstanding for a while and is blocking further work. If you feel it would be more efficient, I would be fine landing this as-is and addressing those points in follow-ups.



================
Comment at: llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:611
+              WasmSym->setType(SymbolType.getValue());
+            }
+          }
----------------
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.


================
Comment at: llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:234-241
+  void Initialize(MCAsmParser &Parser) override {
+    MCAsmParserExtension::Initialize(Parser);
+
+    DefaultFunctionTable = GetOrCreateFunctionTableSymbol(
+        getContext(), "__indirect_function_table");
+    if (!STI->checkFeatures("+reference-types"))
+      DefaultFunctionTable->setOmitFromLinkingSection();
----------------
The code in this patch fixes up the user's assembly by adding in table operands that did not appear in the source. IIUC, the motivation for this is to maintain backward compatibility with existing assembly that does not contain explicit table operands. IMO, rather than magically fixing the user's code, it would be better to error out on any call_indirect without an explicit table operand when reference-types is enabled. The downside would be that users would need to edit their assembly code when they enable reference-types, but I think the benefit of removing this "magic" from the assembler would be worth it.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyFastISel.cpp:872
   } else {
-    // Add placeholders for the type index and immediate flags
+    // Placehoder for the type index.
     MIB.addImm(0);
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90948