[MIRParser] Allow register class names in the form of integer/scalar

[aheejin]

Summary

The current code cannot handle register class names like 'i32', which is a valid register class name in WebAssembly.

When parsing %0:i32, in which i32 is a register class name, this i32 token is lexed here, before calling parseRegisterClassOrBank function. And the token is created here in lexMIToken function. So if the name is i32, it will be categorized as an integer type rather than an identifier type, the prerequisite that this gets checked against register class/bank names.

[aheejin]

@MatzeB Could you take a look? This code fixes a corner case in D22398 (rL292321) you wrote earlier.

[aheejin]

ping

[thegameg]

I see the problem. The solution feels a little hacky though. Would it make more sense to get rid of MIToken::IntegerType and lex i32 to identifier?

[aheejin]

There are cases in which i32 is used to signify a constant type:
Example 1
Example 2
And there are many more. So changing the category of i32 to an identifier will cause all these tests to fail.

[thegameg]

In D45948#1084799, @aheejin wrote:

There are cases in which i32 is used to signify a constant type:
Example 1
Example 2
And there are many more. So changing the category of i32 to an identifier will cause all these tests to fail.

Right. My question was: would it make sense to *always* lex it as identifier and update parseTypedImmediateOperandto also do what we do in maybeLexIntegerOrScalarType.

[aheejin]

• Changed triple name to 'wasm32-unknown-unknown'

[aheejin]

• Removed special handling for integer/scalar/pointer types and merged

[thegameg]

lib/CodeGen/MIRParser/MIParser.cpp

1306–1312

To keep the same behaviour as before, we need to check that the following characters are digits and error out if not. If not, I think this will assert in APInt instead of emitting a parsing error.

[aheejin]

• Check following characters are digits and error out if not

[thegameg]

LGTM with a small refactoring. I would wait until tomorrow for other reviewers to react, if not you can go ahead and commit this.

lib/CodeGen/MIRParser/MIParser.cpp

1307

Can we refactor this into a function?

[aheejin]

• reduced the repeating code to a single line

[aheejin]

Thank you for the reviews!

lib/CodeGen/MIRParser/MIParser.cpp

1307

I reduced the repeating to code to a single line. Would it still be better to factor it out?

[thegameg]

lib/CodeGen/MIRParser/MIParser.cpp

1307

This is even better! Thanks!

[rtereshin]

llvm/trunk/lib/CodeGen/MIRParser/MIParser.cpp

1307 ↗

(On Diff #145354)

This check will pass if the token is a single character s or p identifier, and it probably shouldn't.

1310 ↗

(On Diff #145354)

This doesn't feel right. It was probably supposed to be a single check under the s/p condition, the following copy shouldn't be here, I think.

1342 ↗

(On Diff #145354)

This check will pass if the token is a single character s identifier, and it probably shouldn't.

1357 ↗

(On Diff #145354)

This check will pass if the token is a single character identifier, and it probably shouldn't.

1358 ↗

(On Diff #145354)

This message would be confusing if the first character is not i, s, or p.

[aheejin]

Oh, thank you for pointing out the errors! I fixed them in D46491.

lib/CodeGen/MIRParser/MILexer.h

enum TokenKind {
     md_tbaa,
     md_alias_scope,
     md_noalias,
     md_range,
     md_diexpr,
 
     // Identifier tokens
     Identifier,
-    IntegerType,
     NamedRegister,
     NamedVirtualRegister,
     MachineBasicBlockLabel,
     MachineBasicBlock,
-    PointerType,
-    ScalarType,
     StackObject,
     FixedStackObject,
     NamedGlobalValue,
     GlobalValue,
     ExternalSymbol,
 
     // Other tokens
     IntegerLiteral,

lib/CodeGen/MIRParser/MILexer.cpp

static Cursor lexName(Cursor C, MIToken &Token, MIToken::TokenKind Type,
   }
   while (isIdentifierChar(C.peek()))
     C.advance();
   Token.reset(Type, Range.upto(C))
       .setStringValue(Range.upto(C).drop_front(PrefixLength));
   return C;
 }
 
-static Cursor maybeLexIntegerOrScalarType(Cursor C, MIToken &Token) {
-  if ((C.peek() != 'i' && C.peek() != 's' && C.peek() != 'p') ||
-      !isdigit(C.peek(1)))
-    return None;
-  char Kind = C.peek();
-  auto Range = C;
-  C.advance(); // Skip 'i', 's', or 'p'
-  while (isdigit(C.peek()))
-    C.advance();
-
-  Token.reset(Kind == 'i'
-                  ? MIToken::IntegerType
-                  : (Kind == 's' ? MIToken::ScalarType : MIToken::PointerType),
-              Range.upto(C));
-  return C;
-}
-
 static MIToken::TokenKind getIdentifierKind(StringRef Identifier) {
   return StringSwitch<MIToken::TokenKind>(Identifier)
       .Case("_", MIToken::underscore)
       .Case("implicit", MIToken::kw_implicit)
       .Case("implicit-def", MIToken::kw_implicit_define)
       .Case("def", MIToken::kw_def)
       .Case("dead", MIToken::kw_dead)
       .Case("killed", MIToken::kw_killed)

 StringRef llvm::lexMIToken(StringRef Source, MIToken &Token,
                            ErrorCallbackType ErrorCallback) {
   auto C = skipComment(skipWhitespace(Cursor(Source)));
   if (C.isEOF()) {
     Token.reset(MIToken::Eof, C.remaining());
     return C.remaining();
   }
 
-  if (Cursor R = maybeLexIntegerOrScalarType(C, Token))
-    return R.remaining();
   if (Cursor R = maybeLexMachineBasicBlock(C, Token, ErrorCallback))
     return R.remaining();
   if (Cursor R = maybeLexIdentifier(C, Token))
     return R.remaining();
   if (Cursor R = maybeLexJumpTableIndex(C, Token))
     return R.remaining();
   if (Cursor R = maybeLexStackObject(C, Token))
     return R.remaining();

lib/CodeGen/MIRParser/MIParser.cpp

 bool MIParser::parseIRConstant(StringRef::iterator Loc, const Constant *&C) {
   if (parseIRConstant(Loc, StringRef(Loc, Token.range().end() - Loc), C))
     return true;
   lex();
   return false;
 }
 
 bool MIParser::parseLowLevelType(StringRef::iterator Loc, LLT &Ty) {
-  if (Token.is(MIToken::ScalarType)) {
+  if (Token.range().front() == 's' || Token.range().front() == 'p')
+    if (!llvm::all_of(Token.range().drop_front(), isdigit))
+      return error("Expected integers after 's'/'p' type character");
+  if (!llvm::all_of(Token.range().drop_front(), isdigit))
+    return error("Expected integers after 's'/'p' type character");
+
+  if (Token.range().front() == 's') {
     Ty = LLT::scalar(APSInt(Token.range().drop_front()).getZExtValue());
     lex();
     return false;
-  } else if (Token.is(MIToken::PointerType)) {
+  } else if (Token.range().front() == 'p') {
     const DataLayout &DL = MF.getDataLayout();
     unsigned AS = APSInt(Token.range().drop_front()).getZExtValue();
     Ty = LLT::pointer(AS, DL.getPointerSizeInBits(AS));
     lex();
     return false;
   }
 
   // Now we're looking for a vector.
   if (Token.isNot(MIToken::less))
     return error(Loc,
                  "expected unsized, pN, sN or <N x sM> for GlobalISel type");
 
   lex();
 
   if (Token.isNot(MIToken::IntegerLiteral))
     return error(Loc, "expected <N x sM> for vctor type");
   uint64_t NumElements = Token.integerValue().getZExtValue();
   lex();
 
   if (Token.isNot(MIToken::Identifier) || Token.stringValue() != "x")
     return error(Loc, "expected '<N x sM>' for vector type");
   lex();
 
-  if (Token.isNot(MIToken::ScalarType))
+  if (Token.range().front() != 's')
     return error(Loc, "expected '<N x sM>' for vector type");
+  if (!llvm::all_of(Token.range().drop_front(), isdigit))
+    return error("Expected integers after 's' type character");
   uint64_t ScalarSize = APSInt(Token.range().drop_front()).getZExtValue();
   lex();
 
   if (Token.isNot(MIToken::greater))
     return error(Loc, "expected '<N x sM>' for vector type");
   lex();
 
   Ty = LLT::vector(NumElements, ScalarSize);
   return false;
 }
 
 bool MIParser::parseTypedImmediateOperand(MachineOperand &Dest) {
-  assert(Token.is(MIToken::IntegerType));
+  assert(Token.is(MIToken::Identifier));
+  if (!llvm::all_of(Token.range().drop_front(), isdigit))
+    return error("Expected integers after 'i'/'s'/'p' type character");
+
   auto Loc = Token.location();
   lex();
   if (Token.isNot(MIToken::IntegerLiteral))
     return error("expected an integer literal");
   const Constant *C = nullptr;
   if (parseIRConstant(Loc, C))
     return true;
   Dest = MachineOperand::CreateCImm(cast<ConstantInt>(C));
bool MIParser::parseMachineOperand(MachineOperand &Dest,
   case MIToken::kw_renamable:
   case MIToken::underscore:
   case MIToken::NamedRegister:
   case MIToken::VirtualRegister:
   case MIToken::NamedVirtualRegister:
     return parseRegisterOperand(Dest, TiedDefIdx);
   case MIToken::IntegerLiteral:
     return parseImmediateOperand(Dest);
-  case MIToken::IntegerType:
-    return parseTypedImmediateOperand(Dest);
   case MIToken::kw_half:
   case MIToken::kw_float:
   case MIToken::kw_double:
   case MIToken::kw_x86_fp80:
   case MIToken::kw_fp128:
   case MIToken::kw_ppc_fp128:
     return parseFPImmediateOperand(Dest);
   case MIToken::MachineBasicBlock:
case MIToken::kw_intpred:
     return parsePredicateOperand(Dest);
   case MIToken::Error:
     return true;
   case MIToken::Identifier:
     if (const auto *RegMask = getRegMask(Token.stringValue())) {
       Dest = MachineOperand::CreateRegMask(RegMask);
       lex();
       break;
-    } else
+    } else if (Token.stringValue() == "CustomRegMask") {
       return parseCustomRegisterMaskOperand(Dest);
+    } else
+      return parseTypedImmediateOperand(Dest);
   default:
     // FIXME: Parse the MCSymbol machine operand.
     return error("expected a machine operand");
   }
   return false;
 }
 
 bool MIParser::parseMachineOperandAndTargetFlags(

test/CodeGen/MIR/WebAssembly/int-type-register-class-name.mir

+# RUN: llc -mtriple=wasm32-unknown-unknown -run-pass none -o - %s | FileCheck %s
+# When a register class has a name of an integer type (i32)
+---
+name: regclass_name_test
+liveins:
+  - { reg: '$arguments' }
+body: |
+  bb.0:
+    liveins: $arguments
+    %0:i32 = CONST_I32 0, implicit-def dead $arguments
+    ; CHECK: %0:i32 = CONST_I32 0, implicit-def dead $arguments
+    RETURN_VOID implicit-def dead $arguments
+...

test/CodeGen/MIR/WebAssembly/lit.local.cfg

+if not 'WebAssembly' in config.root.targets:
+    config.unsupported = True