[WebAssembly] made assembler parse block_type

[aardappel]

Summary

This was previously ignored and an incorrect value generated.

Also fixed Disassembler's handling of block_type.

[aardappel]

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.

[aheejin]

In 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 says and what the official spec 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 were changed to single byte values earlier this year by these CLs (D43921, D43922, and D43991).

lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp

303 ↗

(On Diff #179526)

Nit: id should be Id or ID, according to the LLVM coding style

463 ↗

(On Diff #179526)

When do we need this Parser.Lex() and when we don't? This exists only in if (ExpectBlockType) side, so I'm curious.

522 ↗

(On Diff #179526)

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?

lib/Target/WebAssembly/Disassembler/WebAssemblyDisassembler.cpp

176 ↗

(On Diff #179526)

This sentence sounds weird... perhaps a stray 'that'?

lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCTargetDesc.h

350 ↗

(On Diff #179526)

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.

[aardappel]

Addressed code review.

[aardappel]

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

lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp

463 ↗

(On Diff #179526)

You call it whenever you consume a token (it advances to the next token). In the else branch parsePrimaryExpr calls it for us.

522 ↗

(On Diff #179526)

I was originally going to require explicitly writing void, but the existing code (printWebAssemblySignatureOperand) wasn't printing it, so I decided to follow that.

lib/Target/WebAssembly/Disassembler/WebAssemblyDisassembler.cpp

176 ↗

(On Diff #179526)

That comments is incorrect anyway, removed.

lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCTargetDesc.h

350 ↗

(On Diff #179526)

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.

[aardappel]

Ok, decided keeping func/anyfunc out of the enum was cleaner,
as we already have a lot of duplicate enums for subsets of types.

[aheejin]

LGTM with a nit.

lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp

522 ↗

(On Diff #179526)

Yes I agree that we should be able to omit void. What I was suggesting was maybe we could add lines that omit void in the test case too.

[aardappel]

lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp

522 ↗

(On Diff #179526)

There is such a case:
if # void

llvm/trunk/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp

if (Type == "f64")
       return wasm::ValType::F64;
     if (Type == "v128" || Type == "i8x16" || Type == "i16x8" ||
         Type == "i32x4" || Type == "i64x2" || Type == "f32x4" ||
         Type == "f64x2")
       return wasm::ValType::V128;
     return Optional<wasm::ValType>();
   }
 
+  WebAssembly::ExprType parseBlockType(StringRef ID) {
+    return StringSwitch<WebAssembly::ExprType>(ID)
+        .Case("i32", WebAssembly::ExprType::I32)
+        .Case("i64", WebAssembly::ExprType::I64)
+        .Case("f32", WebAssembly::ExprType::F32)
+        .Case("f64", WebAssembly::ExprType::F64)
+        .Case("v128", WebAssembly::ExprType::V128)
+        .Case("except_ref", WebAssembly::ExprType::ExceptRef)
+        .Case("void", WebAssembly::ExprType::Void)
+        .Default(WebAssembly::ExprType::Invalid);
+  }
+
   bool parseRegTypeList(SmallVectorImpl<wasm::ValType> &Types) {
     while (Lexer.is(AsmToken::Identifier)) {
       auto Type = parseType(Lexer.getTok().getString());
       if (!Type)
         return true;
       Types.push_back(Type.getValue());
       Parser.Lex();
       if (!isNext(AsmToken::Comma))
if (IsLoadStore) {
         Operands.push_back(make_unique<WebAssemblyOperand>(
             WebAssemblyOperand::Integer, Offset.getLoc(), Offset.getEndLoc(),
             WebAssemblyOperand::IntOp{0}));
       }
     }
     return false;
   }
 
+  void addBlockTypeOperand(OperandVector &Operands, SMLoc NameLoc,
+                           WebAssembly::ExprType BT) {
+    Operands.push_back(make_unique<WebAssemblyOperand>(
+        WebAssemblyOperand::Integer, NameLoc, NameLoc,
+        WebAssemblyOperand::IntOp{static_cast<int64_t>(BT)}));
+  }
+
   bool ParseInstruction(ParseInstructionInfo & /*Info*/, StringRef Name,
                         SMLoc NameLoc, OperandVector &Operands) override {
     // Note: Name does NOT point into the sourcecode, but to a local, so
     // use NameLoc instead.
     Name = StringRef(NameLoc.getPointer(), Name.size());
 
     // WebAssembly has instructions with / in them, which AsmLexer parses
     // as seperate tokens, so if we find such tokens immediately adjacent (no
Operands.push_back(make_unique<WebAssemblyOperand>(
         WebAssemblyOperand::Token, NameLoc, SMLoc::getFromPointer(Name.end()),
         WebAssemblyOperand::TokOp{Name}));
     auto NamePair = Name.split('.');
     // If no '.', there is no type prefix.
     auto BaseName = NamePair.second.empty() ? NamePair.first : NamePair.second;
 
     // If this instruction is part of a control flow structure, ensure
     // proper nesting.
+    bool ExpectBlockType = false;
     if (BaseName == "block") {
       push(Block);
+      ExpectBlockType = true;
     } else if (BaseName == "loop") {
       push(Loop);
+      ExpectBlockType = true;
     } else if (BaseName == "try") {
       push(Try);
+      ExpectBlockType = true;
     } else if (BaseName == "if") {
       push(If);
+      ExpectBlockType = true;
     } else if (BaseName == "else") {
       if (pop(BaseName, If))
         return true;
       push(Else);
     } else if (BaseName == "catch") {
       if (pop(BaseName, Try))
         return true;
       push(Try);
if (BaseName == "block") {
         return true;
     }
 
     while (Lexer.isNot(AsmToken::EndOfStatement)) {
       auto &Tok = Lexer.getTok();
       switch (Tok.getKind()) {
       case AsmToken::Identifier: {
         auto &Id = Lexer.getTok();
+        if (ExpectBlockType) {
+          // Assume this identifier is a block_type.
+          auto BT = parseBlockType(Id.getString());
+          if (BT == WebAssembly::ExprType::Invalid)
+            return error("Unknown block type: ", Id);
+          addBlockTypeOperand(Operands, NameLoc, BT);
+          Parser.Lex();
+        } else {
+          // Assume this identifier is a label.
           const MCExpr *Val;
           SMLoc End;
           if (Parser.parsePrimaryExpr(Val, End))
             return error("Cannot parse symbol: ", Lexer.getTok());
           Operands.push_back(make_unique<WebAssemblyOperand>(
               WebAssemblyOperand::Symbol, Id.getLoc(), Id.getEndLoc(),
               WebAssemblyOperand::SymOp{Val}));
+        }
         break;
       }
       case AsmToken::Minus:
         Parser.Lex();
         if (Lexer.isNot(AsmToken::Integer))
           return error("Expected integer instead got: ", Lexer.getTok());
         if (parseOperandStartingWithInteger(true, Operands, BaseName))
           return true;
while (Lexer.isNot(AsmToken::EndOfStatement)) {
       default:
         return error("Unexpected token in operand: ", Tok);
       }
       if (Lexer.isNot(AsmToken::EndOfStatement)) {
         if (expect(AsmToken::Comma, ","))
           return true;
       }
     }
-    Parser.Lex();
-
-    // Block instructions require a signature index, but these are missing in
-    // assembly, so we add a dummy one explicitly (since we have no control
-    // over signature tables here, we assume these will be regenerated when
-    // the wasm module is generated).
-    if (BaseName == "block" || BaseName == "loop" || BaseName == "try" ||
-        BaseName == "if") {
-      Operands.push_back(make_unique<WebAssemblyOperand>(
-          WebAssemblyOperand::Integer, NameLoc, NameLoc,
-          WebAssemblyOperand::IntOp{-1}));
+    if (ExpectBlockType && Operands.size() == 1) {
+      // Support blocks with no operands as default to void.
+      addBlockTypeOperand(Operands, NameLoc, WebAssembly::ExprType::Void);
     }
+    Parser.Lex();
     return false;
   }
 
   void onLabelParsed(MCSymbol *Symbol) override {
     LastLabel = Symbol;
     CurrentState = Label;
   }
 

llvm/trunk/lib/Target/WebAssembly/Disassembler/WebAssemblyDisassembler.cpp

for (uint8_t OPI = 0; OPI < WasmInst->NumOperands; OPI++) {
     case WebAssembly::OPERAND_TYPEINDEX:
     case MCOI::OPERAND_IMMEDIATE: {
       if (!parseLEBImmediate(MI, Size, Bytes, false))
         return MCDisassembler::Fail;
       break;
     }
     // SLEB operands:
     case WebAssembly::OPERAND_I32IMM:
-    case WebAssembly::OPERAND_I64IMM:
-    case WebAssembly::OPERAND_SIGNATURE: {
+    case WebAssembly::OPERAND_I64IMM: {
       if (!parseLEBImmediate(MI, Size, Bytes, true))
         return MCDisassembler::Fail;
       break;
     }
+    // block_type operands (uint8_t).
+    case WebAssembly::OPERAND_SIGNATURE: {
+      if (!parseImmediate<uint8_t>(MI, Size, Bytes))
+        return MCDisassembler::Fail;
+      break;
+    }
     // FP operands.
     case WebAssembly::OPERAND_F32IMM: {
       if (!parseImmediate<float>(MI, Size, Bytes))
         return MCDisassembler::Fail;
       break;
     }
     case WebAssembly::OPERAND_F64IMM: {
       if (!parseImmediate<double>(MI, Size, Bytes))

llvm/trunk/lib/Target/WebAssembly/InstPrinter/WebAssemblyInstPrinter.cpp

case WebAssembly::ExprType::F64:
     O << "f64";
     break;
   case WebAssembly::ExprType::V128:
     O << "v128";
     break;
   case WebAssembly::ExprType::ExceptRef:
     O << "except_ref";
     break;
+  default:
+    llvm_unreachable("invalid WebAssembly::ExprType");
   }
 }
 
 const char *llvm::WebAssembly::TypeToString(wasm::ValType Ty) {
   switch (Ty) {
   case wasm::ValType::I32:
     return "i32";
   case wasm::ValType::I64:

llvm/trunk/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCTargetDesc.h

 /// This is used to indicate block signatures.
 enum class ExprType : unsigned {
   Void = 0x40,
   I32 = 0x7F,
   I64 = 0x7E,
   F32 = 0x7D,
   F64 = 0x7C,
   V128 = 0x7B,
-  ExceptRef = 0x68
+  ExceptRef = 0x68,
+  Invalid = 0x00
 };
 
 /// Instruction opcodes emitted via means other than CodeGen.
 static const unsigned Nop = 0x01;
 static const unsigned End = 0x0b;
 
 wasm::ValType toValType(const MVT &Ty);
 
 } // end namespace WebAssembly
 } // end namespace llvm
 
 #endif

llvm/trunk/test/MC/Disassembler/WebAssembly/wasm.txt

 # RUN: llvm-mc --disassemble %s -triple=wasm32-unknown-unknown | FileCheck %s
 
 # CHECK: .text
 
 # CHECK: nop
 0x01
 
 # CHECK: i32.add
 0x6a
 
 # CHECK: i64.const -1
 0x42 0x7F
 
 # CHECK: i64.load32_u 16:p2align=1
 0x35 0x01 0x10
 
-# CHECK: block
-# 3
-# FIXME: WebAssemblyInstPrinter does not currently print block number.
-0x02 0x03
+# CHECK: block f64
+0x02 0x7C
 
 # CHECK: call_indirect
 # $0=, 128, 0
 # FIXME: WebAssemblyInstPrinter does not print immediates.
 0x11 0x80 0x01 0x00
 
 # CHECK: get_local 128
 0x20 0x80 0x01

llvm/trunk/test/MC/WebAssembly/basic-assembly.s

test0:
     i32.const   -1
     f64.const   0x1.999999999999ap1
     v128.const  0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15
     v128.const  0, 1, 2, 3, 4, 5, 6, 7
     # Indirect addressing:
     get_local   0
     f64.store   0
     # Loops, conditionals, binary ops, calls etc:
-    block
+    block       i32
     i32.const   1
     get_local   0
     i32.ge_s
     br_if       0        # 0: down to label0
 .LBB0_1:
-    loop             # label1:
+    loop        i32      # label1:
     call        something1@FUNCTION
     i64.const   1234
     i32.call    something2@FUNCTION
     i32.const   0
     call_indirect 0
     i32.const   1
     i32.add
     tee_local   0
     get_local   0
     i32.lt_s
     br_if       0        # 0: up to label1
 .LBB0_2:
     end_loop
-    end_block                       # label0:
+    end_block            # label0:
     get_local   4
     get_local   5
-    block
-    block
-    block
-    block
+    block       void
+    block       i64
+    block       f32
+    block       f64
     br_table {0, 1, 2}   # 2 entries, default
     end_block            # first entry jumps here.
-    i32.const 1
+    i32.const   1
     br 2
     end_block            # second entry jumps here.
-    i32.const 2
+    i32.const   2
     br 1
     end_block            # default jumps here.
-    i32.const 3
+    i32.const   3
     end_block            # "switch" exit.
-    if
-    if
+    if          # void
+    if          i32
     end_if
     else
     end_if
     f32x4.add
     # Test correct parsing of instructions with / and : in them:
     # TODO: enable once instruction has been added.
     #i32x4.trunc_s/f32x4:sat
     i32.trunc_s/f32
-    try
+    try         except_ref
 .LBB0_3:
     i32.catch   0
 .LBB0_4:
     catch_all
 .LBB0_5:
     end_try
     #i32.trunc_s:sat/f32
     get_global  __stack_pointer@GLOBAL
     end_function
 .Lfunc_end0:
-  .size test0, .Lfunc_end0-test0
+    .size test0, .Lfunc_end0-test0
     .globaltype __stack_pointer, i32
 
 # CHECK:           .text
 # CHECK-LABEL: test0:
 # CHECK-NEXT:      .functype test0 (i32, i64) -> (i32)
 # CHECK-NEXT:      .eventtype  __cpp_exception i32
 # CHECK-NEXT:      .local      f32, f64
 # CHECK-NEXT:      get_local   2
 # CHECK-NEXT:      set_local   2
 # CHECK-NEXT:      i32.const   -1
 # CHECK-NEXT:      f64.const   0x1.999999999999ap1
 # CHECK-NEXT:      v128.const  0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15
 # CHECK-NEXT:      v128.const  0, 1, 2, 3, 4, 5, 6, 7
 # CHECK-NEXT:      get_local   0
 # CHECK-NEXT:      f64.store   0:p2align=0
-# CHECK-NEXT:      block
+# CHECK-NEXT:      block       i32
 # CHECK-NEXT:      i32.const   1
 # CHECK-NEXT:      get_local   0
 # CHECK-NEXT:      i32.ge_s
 # CHECK-NEXT:      br_if 0            # 0: down to label0
 # CHECK-NEXT:  .LBB0_1:
-# CHECK-NEXT:      loop                    # label1:
+# CHECK-NEXT:      loop        i32         # label1:
 # CHECK-NEXT:      call        something1@FUNCTION
 # CHECK-NEXT:      i64.const   1234
 # CHECK-NEXT:      i32.call    something2@FUNCTION
 # CHECK-NEXT:      i32.const   0
 # CHECK-NEXT:      call_indirect 0
 # CHECK-NEXT:      i32.const   1
 # CHECK-NEXT:      i32.add
 # CHECK-NEXT:      tee_local   0
 # CHECK-NEXT:      get_local   0
 # CHECK-NEXT:      i32.lt_s
 # CHECK-NEXT:      br_if 0            # 0: up to label1
 # CHECK-NEXT:  .LBB0_2:
 # CHECK-NEXT:      end_loop
 # CHECK-NEXT:      end_block                       # label0:
 # CHECK-NEXT:      get_local   4
 # CHECK-NEXT:      get_local   5
 # CHECK-NEXT:      block
-# CHECK-NEXT:      block
-# CHECK-NEXT:      block
-# CHECK-NEXT:      block
+# CHECK-NEXT:      block       i64
+# CHECK-NEXT:      block       f32
+# CHECK-NEXT:      block       f64
 # CHECK-NEXT:      br_table {0, 1, 2}  # 1: down to label4
 # CHECK-NEXT:                          # 2: down to label3
 # CHECK-NEXT:      end_block           # label5:
 # CHECK-NEXT:      i32.const 1
 # CHECK-NEXT:      br 2                # 2: down to label2
 # CHECK-NEXT:      end_block           # label4:
 # CHECK-NEXT:      i32.const 2
 # CHECK-NEXT:      br 1                # 1: down to label2
 # CHECK-NEXT:      end_block           # label3:
 # CHECK-NEXT:      i32.const 3
 # CHECK-NEXT:      end_block           # label2:
 # CHECK-NEXT:      if
-# CHECK-NEXT:      if
+# CHECK-NEXT:      if          i32
 # CHECK-NEXT:      end_if
 # CHECK-NEXT:      else
 # CHECK-NEXT:      end_if
 # CHECK-NEXT:      f32x4.add
 # CHECK-NEXT:      i32.trunc_s/f32
-# CHECK-NEXT:      try
+# CHECK-NEXT:      try         except_ref
 # CHECK-NEXT:  .LBB0_3:
 # CHECK-NEXT:      i32.catch   0
 # CHECK-NEXT:  .LBB0_4:
 # CHECK-NEXT:      catch_all
 # CHECK-NEXT:  .LBB0_5:
 # CHECK-NEXT:      end_try
 # CHECK-NEXT:      get_global  __stack_pointer@GLOBAL
 # CHECK-NEXT:      end_function
 
 # CHECK:           .globaltype  __stack_pointer, i32