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

[Thomas Lively via Phabricator via llvm-commits]

tlively marked 4 inline comments as done.
tlively added inline comments.


================
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) {
----------------
aheejin wrote:
> What are septet values, and when is `Val` negative? All values of `BlockType` look unsigned. Maybe I'm missing something..?
See https://webassembly.github.io/multi-value/core/binary/instructions.html#control-instructions for details on the encoding. By `septet value` I just mean a group of 7 bits. That's where the `& 0x7f` comes from.


================
Comment at: llvm/lib/Target/WebAssembly/Disassembler/WebAssemblyDisassembler.cpp:227
+          MI.addOperand(
+              MCOperand::createImm(int64_t(WebAssembly::BlockType::Invalid)));
+        } else {
----------------
aheejin wrote:
> When does this happen?
See wasm-error.txt for an example. Basically anytime you have a negative SLEB128 value here that occupies more than one byte, which is invalid according to the spec.


================
Comment at: llvm/lib/Target/WebAssembly/Disassembler/WebAssemblyDisassembler.cpp:239
+        MI.addOperand(MCOperand::createExpr(Expr));
+      }
       break;
----------------
aheejin wrote:
> Disassembler is not going to print multivalue signatures then? Is this tentative or permanent?
I don't think there's any way to access the type section from the disassembler, so this is permanent unless there's a large overhaul. cc @aardappel for the details.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp:1248
+      case WebAssembly::END_BLOCK:
+      case WebAssembly::END_LOOP:
         EndToBegin[&MI]->getOperand(0).setImm(int32_t(RetType));
----------------
aheejin wrote:
> It's preexisting, but I think we should add `END_TRY` here too. Not sure if I can generate a test case that ends with `end_try` returning something easily though..
Sounds good.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68889