This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by aheejin on Apr 23 2018, 6:26 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

aheejin created this revision.Apr 23 2018, 6:26 AM
aheejin edited the summary of this revision. (Show Details)Apr 23 2018, 6:55 AM

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

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?

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.

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 updated this revision to Diff 144854.May 2 2018, 4:38 AM
  • Changed triple name to 'wasm32-unknown-unknown'
aheejin updated this revision to Diff 144972.May 2 2018, 6:51 PM
  • Removed special handling for integer/scalar/pointer types and merged
thegameg added inline comments.May 3 2018, 1:09 AM
lib/CodeGen/MIRParser/MIParser.cpp
1283 ↗(On Diff #144972)

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 updated this revision to Diff 145002.May 3 2018, 4:42 AM
  • Check following characters are digits and error out if not
thegameg accepted this revision.May 3 2018, 5:07 AM

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 ↗(On Diff #145002)

Can we refactor this into a function?

This revision is now accepted and ready to land.May 3 2018, 5:07 AM
aheejin updated this revision to Diff 145093.May 3 2018, 2:21 PM
  • reduced the repeating code to a single line
aheejin marked an inline comment as done.May 3 2018, 2:23 PM

Thank you for the reviews!

lib/CodeGen/MIRParser/MIParser.cpp
1307 ↗(On Diff #145002)

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

thegameg added inline comments.May 3 2018, 2:43 PM
lib/CodeGen/MIRParser/MIParser.cpp
1307 ↗(On Diff #145002)

This is even better! Thanks!

This revision was automatically updated to reflect the committed changes.
rtereshin added inline comments.
llvm/trunk/lib/CodeGen/MIRParser/MIParser.cpp
1307

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

1310

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

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

1357

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

1358

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

aheejin marked 5 inline comments as done.May 5 2018, 3:09 AM

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