[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

1291

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

1292

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

1292

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

1292

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/MIParser.cpp

case MIToken::VirtualRegister:
     return false;
   // TODO: Parse other register kinds.
   default:
     llvm_unreachable("The current token should be a register");
   }
 }
 
 bool MIParser::parseRegisterClassOrBank(VRegInfo &RegInfo) {
+  // It is possible a register class or register bank name can be in the form of
+  // integer of scalar type (e.g. i32). Fix token kind in that case.
+  auto Range = Token.range();
+  if (Token.isNot(MIToken::underscore) &&
+      (PFS.Names2RegClasses.find(Range) != PFS.Names2RegClasses.end() ||
+       PFS.Names2RegBanks.find(Range) != PFS.Names2RegBanks.end()))
+    Token.reset(MIToken::Identifier, Range).setStringValue(Range);
+
   if (Token.isNot(MIToken::Identifier) && Token.isNot(MIToken::underscore))
     return error("expected '_', register class, or register bank name");
   StringRef::iterator Loc = Token.location();
-  StringRef Name = Token.stringValue();
 
   // Was it a register class?
+  StringRef Name = Token.stringValue();
   auto RCNameI = PFS.Names2RegClasses.find(Name);
   if (RCNameI != PFS.Names2RegClasses.end()) {
     lex();
     const TargetRegisterClass &RC = *RCNameI->getValue();
 
     switch (RegInfo.Kind) {
     case VRegInfo::UNKNOWN:
     case VRegInfo::NORMAL:

 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)) {
     Ty = LLT::scalar(APSInt(Token.range().drop_front()).getZExtValue());
     lex();
     return false;
   } else if (Token.is(MIToken::PointerType)) {
     const DataLayout &DL = MF.getDataLayout();
     unsigned AS = APSInt(Token.range().drop_front()).getZExtValue();
     Ty = LLT::pointer(AS, DL.getPointerSizeInBits(AS));
     lex();
     return false;

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