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

[Wouter van Oortmerssen via Phabricator via llvm-commits]

aardappel added a comment.

@aheejin thanks for pointing out the spec discrepancy, I've now made the disassembler explicitly parse a byte instead of a SLEB for consistency.



================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:463
+          addBlockTypeOperand(Operands, NameLoc, BT);
+          Parser.Lex();
+        } else {
----------------
aheejin wrote:
> When do we need this `Parser.Lex()` and when we don't? This exists only in `if (ExpectBlockType)` side, so I'm curious.
You call it whenever you consume a token (it advances to the next token). In the else branch `parsePrimaryExpr` calls it for us.


================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:522
+      // Support blocks with no operands as default to void.
+      addBlockTypeOperand(Operands, NameLoc, WebAssembly::ExprType::Void);
     }
----------------
aheejin wrote:
> 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?
I was originally going to require explicitly writing `void`, but the existing code (printWebAssemblySignatureOperand) wasn't printing it, so I decided to follow that.


================
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: {
----------------
aheejin wrote:
> This sentence sounds weird... perhaps a stray 'that'?
That comments is incorrect anyway, removed.


================
Comment at: lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCTargetDesc.h:350
+  AnyFunc = 0x70,
+  Func = 0x60,
+  ExceptRef = 0x68,
----------------
aheejin wrote:
> 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.
Got that from https://github.com/WebAssembly/design/blob/master/BinaryEncoding.md#language-types

Ok, will disable them in the assembler block_type parsing. Will leave them in the enum, since they are part of the set of types we have, and if anyone refers to them it will likely be through this enum.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56092