[PATCH] D68889: [WebAssembly] Allow multivalue types in block signature operands

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added a comment.

Nice! Mostly LGTM.

> Currently non-void blocks are only generated at the end of functions where the block return type needs to agree with the function return type, and that remains true for multivalue blocks. That invariant means that the actual signature does not need to be stored in the block signature MachineOperand because it can be inferred by WebAssemblyMCInstLower from the return type of the parent function.

I guess this is a tentative state before you implement the rest of the proposal in full, right? If other blocks are able to return multivalue, are we gonna change their operands to also take typeindex?



================
Comment at: llvm/lib/Target/WebAssembly/Disassembler/WebAssemblyDisassembler.cpp:224
+      if (Val < 0) {
+        // Negative values are single septet value types or empty types
+        if (Size != PrevSize + 1) {
----------------
What are septet values, and when is `Val` negative? All values of `BlockType` look unsigned. Maybe I'm missing something..?


================
Comment at: llvm/lib/Target/WebAssembly/Disassembler/WebAssemblyDisassembler.cpp:227
+          MI.addOperand(
+              MCOperand::createImm(int64_t(WebAssembly::BlockType::Invalid)));
+        } else {
----------------
When does this happen?


================
Comment at: llvm/lib/Target/WebAssembly/Disassembler/WebAssemblyDisassembler.cpp:239
+        MI.addOperand(MCOperand::createExpr(Expr));
+      }
       break;
----------------
Disassembler is not going to print multivalue signatures then? Is this tentative or permanent?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp:203
+  computeLegalValueVTs(F, TM, RetTy, CallerRetTys);
+  llvm::valTypesFromMVTs(CallerRetTys, Returns);
+}
----------------
Nit: Do we need `llvm::` here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68889