[PATCH] D56092: [WebAssembly] made assembler parse block_type

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added a comment.

In D56092#1341055 <https://reviews.llvm.org/D56092#1341055>, @aardappel wrote:

> Note to reviewers: I can fix the FIXME in the disassembler by making it a proper SLEB, and change the WebAssembly::ExprType enum to have negative values as per the spec. I think that should have little impact as the backend treats these values as opaque, afaik.. though maybe I am missing something. Happy to put that in this CL or a next one.


I think there's a discrepancy between what the github binary encoding doc <https://github.com/WebAssembly/design/blob/master/BinaryEncoding.md#language-types> says and what the official spec <https://webassembly.github.io/spec/core/binary/types.html#value-types> says. The official spec lists them explicitly as single byte values and not LEB, and I think we should follow the official spec. Actually, they had been listed as signed values but changed to single byte values earlier this year by these CLs (CL1 <https://reviews.llvm.org/D43921>,  CL2 <https://reviews.llvm.org/D43922>, and CL3 <https://reviews.llvm.org/D43991>).



================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:303
 
+  WebAssembly::ExprType parseBlockType(StringRef id) {
+    return StringSwitch<WebAssembly::ExprType>(id)
----------------
Nit: `id` should be `Id` or `ID`, according to the LLVM coding style


================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:463
+          addBlockTypeOperand(Operands, NameLoc, BT);
+          Parser.Lex();
+        } else {
----------------
When do we need this `Parser.Lex()` and when we don't? This exists only in `if (ExpectBlockType)` side, so I'm curious.


================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:522
+      // Support blocks with no operands as default to void.
+      addBlockTypeOperand(Operands, NameLoc, WebAssembly::ExprType::Void);
     }
----------------
It looks like we assume no signature as void, but all lines in the test case now have type signatures. Can we also have block/loop/try with no signatures?


================
Comment at: lib/Target/WebAssembly/Disassembler/WebAssemblyDisassembler.cpp:176
+    // FIXME: SLEB operand (according to spec) that is stored as uint8_t in
+    // MCInst.
+    case WebAssembly::OPERAND_SIGNATURE: {
----------------
This sentence sounds weird... perhaps a stray 'that'?


================
Comment at: lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCTargetDesc.h:350
+  AnyFunc = 0x70,
+  Func = 0x60,
+  ExceptRef = 0x68,
----------------
What is the difference between anyfunc and func?
Anyway, the current spec says [[ https://github.com/WebAssembly/design/blob/master/BinaryEncoding.md#block_type | `block_type` ]] should be either one of [[ https://github.com/WebAssembly/design/blob/master/BinaryEncoding.md#value_type | `value_type` ]] s or void, so I think that means anyfunc or func should not be allowed as a block type. [[ https://github.com/WebAssembly/exception-handling/blob/master/proposals/Exceptions.md#value_type | `except_ref` should be allowed ]] as a value type for the EH proposal though.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56092