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

[Groverkss]

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.

[Groverkss]

• Remove debug header

[rriddle]

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)

[rriddle]

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]

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]

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.

[rriddle]

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]

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]

• Added doc comments + tests
• Fixed some bugs found while adding tests

[Groverkss]

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]

• Fix small bug in previous implementation

[bondhugula]

In D127076#3561425, @Groverkss wrote:

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

365

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

[Groverkss]

Address Uday's comments and rebase

[Groverkss]

@rriddle gentle ping :)

[Groverkss]

@rriddle ping

[rriddle]

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

mlir/lib/Parser/AffineParser.cpp

AffineExpr AffineParser::parseNegateExpression(AffineExpr lhs) {
   // parseAffineOperandExpr instead of parseAffineExpr here.
   if (!operand)
     // Extra error message although parseAffineOperandExpr would have
     // complained. Leads to a better diagnostic.
     return emitError("missing operand of negation"), nullptr;
   return (-1) * operand;
 }
 
+static bool isBareIdentifier(const Token &token) {
+    return token.isAny(Token::bare_identifier, Token::inttype) ||
+           token.isKeyword();
+}
+
 /// Parse a bare id that may appear in an affine expression.
 ///
 ///   affine-expr ::= bare-id
 AffineExpr AffineParser::parseBareIdExpr() {
-  if (getToken().isNot(Token::bare_identifier))
+  if (!isBareIdentifier(getToken()))
     return emitWrongTokenError("expected bare identifier"), nullptr;
 
   StringRef sRef = getTokenSpelling();
   for (auto entry : dimsAndSymbols) {
     if (entry.first == sRef) {
-      consumeToken(Token::bare_identifier);
+      consumeToken();
       return entry.second;
     }
   }
 
   return emitWrongTokenError("use of undeclared identifier"), nullptr;
 }
 
 /// Parse an SSA id which may appear in an affine expression.
case Token::integer:
     return parseIntegerExpr();
   case Token::l_paren:
     return parseParentheticalExpr();
   case Token::minus:
     return parseNegateExpression(lhs);
   case Token::kw_ceildiv:
   case Token::kw_floordiv:
   case Token::kw_mod:
   case Token::plus:
   case Token::star:
     if (lhs)
       emitError("missing right operand of binary operator");
     else
       emitError("missing left operand of binary operator");
     return nullptr;
   default:
     if (lhs)

 AffineExpr AffineParser::parseAffineExpr() {
   return parseAffineLowPrecOpExpr(nullptr, AffineLowPrecOp::LNoOp);
 }
 
 /// Parse a dim or symbol from the lists appearing before the actual
 /// expressions of the affine map. Update our state to store the
 /// dimensional/symbolic identifier.
 ParseResult AffineParser::parseIdentifierDefinition(AffineExpr idExpr) {
-  if (getToken().isNot(Token::bare_identifier))
+  if (!isBareIdentifier(getToken()))
     return emitWrongTokenError("expected bare identifier");
 
   auto name = getTokenSpelling();
   for (auto entry : dimsAndSymbols) {
     if (entry.first == name)
       return emitError("redefinition of identifier '" + name + "'");
   }
-  consumeToken(Token::bare_identifier);
+  consumeToken();
 
   dimsAndSymbols.push_back({name, idExpr});
   return success();
 }
 
 /// Parse the list of dimensional identifiers to an affine map.
 ParseResult AffineParser::parseDimIdList(unsigned &numDims) {
   auto parseElt = [&]() -> ParseResult {