This is an archive of the discontinued LLVM Phabricator instance.

[MLIR][Parser] Fix AffineParser colliding bare identifiers with primitive types
ClosedPublic

Authored by Groverkss on Jun 5 2022, 2:13 PM.

Details

Summary

The parser currently can't parse bare identifiers like 'i0' in affine
maps and sets, and similarly ids like f16/f32. But these bare ids are
part of the grammar - although they are primitive types.

error: expected bare identifier
set = affine_set<(i0, i1) : ()>
                   ^

This patch allows the parser for AffineMap/IntegerSet to parse bare
identifiers as defined by the grammer.

Diff Detail

Event Timeline

Groverkss created this revision.Jun 5 2022, 2:13 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 5 2022, 2:13 PM
Groverkss requested review of this revision.Jun 5 2022, 2:13 PM
Groverkss updated this revision to Diff 434354.Jun 5 2022, 2:14 PM
  • Remove debug header
rriddle requested changes to this revision.Jun 5 2022, 4:14 PM
rriddle added inline comments.
mlir/lib/Parser/Token.cpp
193–203 ↗(On Diff #434354)

We should really avoid this, this function is effectively re-lexing the token. Take a look at what the AsmParserImpl does, we should do the same thing (which shouldn't require touch the Token class)

This revision now requires changes to proceed.Jun 5 2022, 4:14 PM
rriddle added inline comments.Jun 5 2022, 4:18 PM
mlir/lib/Parser/Token.cpp
193–203 ↗(On Diff #434354)

More specifically, the isCurrentTokenAKeyword function which checks for the tokens that could be treated as a "keyword".

Groverkss updated this revision to Diff 434359.Jun 5 2022, 5:04 PM
Groverkss marked 2 inline comments as done.

Use implementation of isCurrentTokenAKeyword from AsmParserImpl to check
for bare identifier token and make it a static function instead of modifying the
Token class.

bondhugula requested changes to this revision.Jun 5 2022, 6:55 PM
bondhugula added inline comments.
mlir/lib/Parser/AffineParser.cpp
268–271

This can't be the right approach. There is nothing special about inttype; you could have the same issue with floattype. This is also missing doc and code comments.

A test case is missing as well.

This revision now requires changes to proceed.Jun 5 2022, 6:55 PM
rriddle added inline comments.Jun 5 2022, 7:02 PM
mlir/lib/Parser/AffineParser.cpp
268–271

There is something special in that inttype is the only non-keyword token (aside from bare_identifier). You would not run into this for floattype because there is no floattype token, there are various float type "keywords" which are covered by the isKeyword call on Token.

bondhugula added inline comments.Jun 5 2022, 8:58 PM
mlir/lib/Parser/AffineParser.cpp
268–271

I see, thanks.

It looks odd though here to define a bare identifier as either a bare identifier or an int type or a keyword.

Groverkss updated this revision to Diff 434580.Jun 6 2022, 1:25 PM
  • Added doc comments + tests
  • Fixed some bugs found while adding tests
Groverkss added a comment.EditedJun 6 2022, 1:28 PM

While adding tests, I noticed affine_map's like:

#map = affine_map<(d0, d1)[mod] -> (d0 + d1 + mod)>

wouldn't be parsed, even though they are correct according to the grammar. I added support to parse these affine_maps too.

Groverkss updated this revision to Diff 434595.Jun 6 2022, 1:40 PM
  • Fix small bug in previous implementation
bondhugula accepted this revision.Jun 13 2022, 4:56 AM

While adding tests, I noticed affine_map's like:

#map = affine_map<(d0, d1)[mod] -> (d0 + d1 + mod)>

wouldn't be parsed, even though they are correct according to the grammar. I added support to parse these affine_maps too.

Interesting; that's good. I'll defer to @rriddle for the review here given his earlier comments. Dismissing my change requests.

mlir/lib/Parser/AffineParser.cpp
366

Nit: this token ... -> these tokens ... as identifiers

Groverkss marked an inline comment as done.

Address Uday's comments and rebase

rriddle accepted this revision.Jun 27 2022, 9:28 AM

LG (I've been OOO for the past few weeks)

This revision is now accepted and ready to land.Jun 27 2022, 9:28 AM
This revision was landed with ongoing or failed builds.Jun 27 2022, 11:44 AM
This revision was automatically updated to reflect the committed changes.