[WebAssembly] Fixed AsmParser not allowing instructions with / and :

[aardappel]

Summary

The AsmParser Lexer regards these as seperate token.
Here we expand the instruction name with them if they are
adjacent (no whitespace).

Tested: the basic-assembly.s test case has one case with a / in it.
The case with a : needs to be enabled once the corresponding
instruction is available, but has been tested to work in the debugger.

[tlively]

test/MC/WebAssembly/basic-assembly.s

48

What happens if an instruction name is used as a label? Is the colon correctly identified as delimiting a label and not as part of the instruction? Is that even allowed?

[dschuff]

test/MC/WebAssembly/basic-assembly.s

47

We already have support for i32.trunc_s:sat/f32 (non-vector) and friends behind the FeatureNontrappingFPToInt CPU feature. So we should be able to add that to this test already.

[aardappel]

Turns out i32.trunc_s:sat/f32 is parsed as a label i32.trunc_s: followed by an instruction sat/f32.

The label parsing happens in the target agnostic code.

A simple solution would be to require whitespace after :, but only for the wasm target? Or a bit hairier, add a method that asks the target, is this a valid label? (so we can check against a list of instructions) ?

Best would of course be to not have : in names in the first place.

[tlively]

I don't think it's too late to remove colons from all instruction names, so lets try to push in that direction. It's good that this works out cleanly enough for slashes, though.

[aardappel]

Do we want to merge this mean-while to gain / support?

[aheejin]

Is it possible to fix target-independent parsing code to recognize colons in names?

[dschuff]

This comment was removed by dschuff.

[dschuff]

I think it makes the most sense to try to remove colons from instruction names in the spec. Only a few shipped instructions have them. Slashes is probably harder, there are a lot more of those and they are all from the original MVP and not from nontrappingFP. So we can probably just remove colon support from this CL and land it.

[aardappel]

@dschuff ok, will do.
@aheejin that is possible, but would affect other targets, unless we have some WebAssembly conditional code in there. Probably nice to solve in entirely on the WebAssembly side.

[aardappel]

Addressed review comments, now only fixing / in names.

lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp

if (IsLoadStore) {
             WebAssemblyOperand::IntOp{0}));
       }
     }
     return false;
   }
 
   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
+    // whitespace), expand the name to include them:
+    for (;;) {
+      auto &Sep = Lexer.getTok();
+      if (Sep.getLoc().getPointer() != Name.end() ||
+          Sep.getKind() != AsmToken::Slash) break;
+      // Extend name with /
+      Name = StringRef(Name.begin(), Name.size() + Sep.getString().size());
+      Parser.Lex();
+      // We must now find another identifier, or error.
+      auto &Id = Lexer.getTok();
+      if (Id.getKind() != AsmToken::Identifier ||
+          Id.getLoc().getPointer() != Name.end())
+        return Error("Incomplete instruction name: ", Id);
+      Name = StringRef(Name.begin(), Name.size() + Id.getString().size());
+      Parser.Lex();
+    }
+    // Now construct the name as first operand.
     Operands.push_back(make_unique<WebAssemblyOperand>(
-        WebAssemblyOperand::Token, NameLoc,
-        SMLoc::getFromPointer(NameLoc.getPointer() + Name.size()),
-        WebAssemblyOperand::TokOp{
-            StringRef(NameLoc.getPointer(), Name.size())}));
+        WebAssemblyOperand::Token, NameLoc, SMLoc::getFromPointer(Name.end()),
+        WebAssemblyOperand::TokOp{Name}));
     auto NamePair = Name.split('.');
     // If no '.', there is no type prefix.
-    if (NamePair.second.empty())
-      std::swap(NamePair.first, NamePair.second);
+    auto BaseName = NamePair.second.empty() ? NamePair.first : NamePair.second;
     while (Lexer.isNot(AsmToken::EndOfStatement)) {
       auto &Tok = Lexer.getTok();
       switch (Tok.getKind()) {
       case AsmToken::Identifier: {
         auto &Id = Lexer.getTok();
         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, NamePair.second))
+        if (ParseOperandStartingWithInteger(true, Operands, BaseName))
           return true;
         break;
       case AsmToken::Integer:
-        if (ParseOperandStartingWithInteger(false, Operands, NamePair.second))
+        if (ParseOperandStartingWithInteger(false, Operands, BaseName))
           return true;
         break;
       case AsmToken::Real: {
         double Val;
         if (Tok.getString().getAsDouble(Val, false))
           return Error("Cannot parse real: ", Tok);
         Operands.push_back(make_unique<WebAssemblyOperand>(
             WebAssemblyOperand::Float, Tok.getLoc(), Tok.getEndLoc(),
while (Lexer.isNot(AsmToken::EndOfStatement)) {
           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 (NamePair.second == "block" || NamePair.second == "loop") {
+    if (BaseName == "block" || BaseName == "loop") {
       Operands.push_back(make_unique<WebAssemblyOperand>(
           WebAssemblyOperand::Integer, NameLoc, NameLoc,
           WebAssemblyOperand::IntOp{-1}));
     }
     return false;
   }
 
   void onLabelParsed(MCSymbol *Symbol) override { LastLabel = Symbol; }

test/MC/WebAssembly/basic-assembly.s

-# RUN: llvm-mc -triple=wasm32-unknown-unknown -mattr=+sign_ext,+simd128 < %s | FileCheck %s
+# RUN: llvm-mc -triple=wasm32-unknown-unknown -mattr=+simd128,+nontrapping-fptoint < %s | FileCheck %s
 
     .text
     .type    test0,@function
 test0:
     # Test all types:
     .param      i32, i64
     .local      f32, f64, v128, v128
     # Explicit getlocal/setlocal:
.LBB0_1:
     i32.lt_s
     br_if       0        # 0: up to label1
 .LBB0_2:
     end_loop
     end_block                       # label0:
     get_local   4
     get_local   5
     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
+    #i32.trunc_s:sat/f32
     end_function
 
 
 # CHECK:           .text
 # CHECK-LABEL: test0:
 # CHECK-NEXT:      .param      i32, i64
 # CHECK-NEXT:      .local      f32, f64
 # CHECK-NEXT:      get_local   2

 # 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:      f32x4.add
+# CHECK-NEXT:      i32.trunc_s/f32
 # CHECK-NEXT:      end_function