fix: address comments from reviewers.

Author: ttjost

mlir/lib/Tools/PDLL/Parser/Lexer.cpp

@@ -329,7 +329,6 @@ Token Lexer::lexIdentifier(const char *tokStart) {
                          .Case("Value", Token::kw_Value)
                          .Case("ValueRange", Token::kw_ValueRange)
                          .Case("with", Token::kw_with)
-                         .Case("array", Token::kw_Array)
                          .Case("_", Token::underscore)
                          .Default(Token::identifier);
   return Token(kind, str);
@@ -379,7 +378,7 @@ Token Lexer::lexString(const char *tokStart, bool isStringBlock) {
       --curPtr;
 
       StringRef expectedEndStr = isStringBlock ? "}]" : "\"";
-      return emitError(curPtr - 1,
+      return emitError(tokStart,
                        "expected '" + expectedEndStr + "' in string literal");
     }
 

mlir/lib/Tools/PDLL/Parser/Lexer.h

@@ -54,7 +54,6 @@ class Token {
 
     /// General keywords.
     kw_Attr,
-    kw_Array,
     kw_erase,
     kw_let,
     kw_Constraint,

mlir/lib/Tools/PDLL/Parser/Parser.cpp

@@ -314,9 +314,11 @@ class Parser {
   FailureOr<ast::Expr *> parseExpr();
 
   /// Identifier expressions.
+  FailureOr<ast::Expr *> parseArrayAttrExpr();
   FailureOr<ast::Expr *> parseAttributeExpr();
   FailureOr<ast::Expr *> parseCallExpr(ast::Expr *parentExpr);
   FailureOr<ast::Expr *> parseDeclRefExpr(StringRef name, SMRange loc);
+  FailureOr<ast::Expr *> parseDictExpr();
   FailureOr<ast::Expr *> parseIdentifierExpr();
   FailureOr<ast::Expr *> parseInlineConstraintLambdaExpr();
   FailureOr<ast::Expr *> parseInlineRewriteLambdaExpr();
@@ -329,8 +331,6 @@ class Parser {
   FailureOr<ast::Expr *> parseTupleExpr();
   FailureOr<ast::Expr *> parseTypeExpr();
   FailureOr<ast::Expr *> parseUnderscoreExpr();
-  FailureOr<ast::Expr *> parseDictExpr();
-  FailureOr<ast::Expr *> parseArrayAttrExpr();
   //===--------------------------------------------------------------------===//
   // Stmts
 
@@ -414,6 +414,13 @@ class Parser {
   FailureOr<ast::MemberAccessExpr *>
   createMemberAccessExpr(ast::Expr *parentExpr, StringRef name, SMRange loc);
 
+  // Create a native call with \p nativeFuncName and \p arguments.
+  // This should be accompanied by a C++ implementation of the function that
+  // needs to be linked and registered in passes that process PDLL files.
+  FailureOr<ast::DeclRefExpr *>
+  createNativeCall(SMRange loc, StringRef nativeFuncName,
+                   MutableArrayRef<ast::Expr *> arguments);
+
   /// Validate the member access `name` into the given parent expression. On
   /// success, this also returns the type of the member accessed.
   FailureOr<ast::Type> validateMemberAccess(ast::Expr *parentExpr,
@@ -443,9 +450,6 @@ class Parser {
   FailureOr<ast::TupleExpr *> createTupleExpr(SMRange loc,
                                               ArrayRef<ast::Expr *> elements,
                                               ArrayRef<StringRef> elementNames);
-  FailureOr<ast::DeclRefExpr *>
-  createNativeCall(SMRange loc, StringRef nativeFuncName,
-                   MutableArrayRef<ast::Expr *> arguments);
 
   //===--------------------------------------------------------------------===//
   // Stmts
@@ -1822,9 +1826,12 @@ FailureOr<ast::Expr *> Parser::parseExpr() {
   case Token::l_brace:
     lhsExpr = parseDictExpr();
     break;
-  case Token::kw_Array:
+  case Token::l_square:
     lhsExpr = parseArrayAttrExpr();
     break;
+  case Token::string_block:
+    return emitError("expected expression. If you are trying to create an "
+                     "ArrayAttr, use a space between `[` and `{`.");
   default:
     return emitError("expected expression");
   }
@@ -1848,6 +1855,40 @@ FailureOr<ast::Expr *> Parser::parseExpr() {
   }
 }
 
+FailureOr<ast::Expr *> Parser::parseArrayAttrExpr() {
+
+  consumeToken(Token::l_square);
+
+  if (parserContext != ParserContext::Rewrite)
+    return emitError(
+        "Parsing of array attributes as constraint not supported!");
+
+  auto arrayAttrCall =
+      createNativeCall(curToken.getLoc(), "createArrayAttr", {});
+  if (failed(arrayAttrCall))
+    return failure();
+
+  do {
+    FailureOr<ast::Expr *> attr = parseExpr();
+    if (failed(attr))
+      return failure();
+
+    SmallVector<ast::Expr *> arrayAttrArgs{*arrayAttrCall, *attr};
+    auto elemToArrayCall = createNativeCall(
+        curToken.getLoc(), "addElemToArrayAttr", arrayAttrArgs);
+    if (failed(elemToArrayCall))
+      return failure();
+
+    // Uses the new array for the next element.
+    arrayAttrCall = elemToArrayCall;
+  } while (consumeIf(Token::comma));
+
+  if (failed(
+          parseToken(Token::r_square, "expected `]` to close array attribute")))
+    return failure();
+  return arrayAttrCall;
+}
+
 FailureOr<ast::Expr *> Parser::parseAttributeExpr() {
   SMRange loc = curToken.getLoc();
   consumeToken(Token::kw_attr);
@@ -1906,6 +1947,62 @@ FailureOr<ast::Expr *> Parser::parseDeclRefExpr(StringRef name, SMRange loc) {
   return createDeclRefExpr(loc, decl);
 }
 
+FailureOr<ast::Expr *> Parser::parseDictExpr() {
+  consumeToken(Token::l_brace);
+  SMRange loc = curToken.getLoc();
+
+  if (parserContext != ParserContext::Rewrite)
+    return emitError(
+        "Parsing of dictionary attributes as constraint not supported!");
+
+  auto dictAttrCall = createNativeCall(loc, "createDictionaryAttr", {});
+  if (failed(dictAttrCall))
+    return failure();
+
+  // Add each nested attribute to the dict
+  do {
+    FailureOr<ast::NamedAttributeDecl *> decl =
+        parseNamedAttributeDecl(std::nullopt);
+    if (failed(decl))
+      return failure();
+
+    ast::NamedAttributeDecl *namedDecl = *decl;
+
+    std::string stringAttrValue =
+        "\"" + std::string((*namedDecl).getName().getName()) + "\"";
+    auto *stringAttr = ast::AttributeExpr::create(ctx, loc, stringAttrValue);
+
+    // Declare it as a variable
+    std::string anonName =
+        llvm::formatv("dict{0}", anonymousDeclNameCounter++).str();
+    FailureOr<ast::VariableDecl *> stringAttrDecl =
+        createVariableDecl(anonName, namedDecl->getLoc(), stringAttr, {});
+    if (failed(stringAttrDecl))
+      return failure();
+
+    // Get its reference
+    auto stringAttrRef = parseDeclRefExpr(
+        (*stringAttrDecl)->getName().getName(), namedDecl->getLoc());
+    if (failed(stringAttrRef))
+      return failure();
+
+    // Create addEntryToDictionaryAttr native call.
+    SmallVector<ast::Expr *> arrayAttrArgs{*dictAttrCall, *stringAttrRef,
+                                           namedDecl->getValue()};
+    auto entryToDictionaryCall =
+        createNativeCall(loc, "addEntryToDictionaryAttr", arrayAttrArgs);
+    if (failed(entryToDictionaryCall))
+      return failure();
+
+    // Uses the new array for the next element.
+    dictAttrCall = entryToDictionaryCall;
+  } while (consumeIf(Token::comma));
+  if (failed(parseToken(Token::r_brace,
+                        "expected `}` to close dictionary attribute")))
+    return failure();
+  return dictAttrCall;
+}
+
 FailureOr<ast::Expr *> Parser::parseIdentifierExpr() {
   StringRef name = curToken.getSpelling();
   SMRange nameLoc = curToken.getLoc();
@@ -2255,114 +2352,6 @@ FailureOr<ast::Expr *> Parser::parseUnderscoreExpr() {
   return createInlineVariableExpr(type, name, nameLoc, constraints);
 }
 
-FailureOr<ast::Expr *> Parser::parseDictExpr() {
-  consumeToken(Token::l_brace);
-  SMRange loc = curToken.getLoc();
-
-  if (parserContext != ParserContext::Rewrite)
-    return emitError(
-        "Parsing of dictionary attributes as constraint not supported!");
-
-  auto dictAttrCall = createNativeCall(loc, "createDictionaryAttr", {});
-  if (failed(dictAttrCall))
-    return failure();
-
-  // Add each nested attribute to the dict
-  do {
-    FailureOr<ast::NamedAttributeDecl *> decl =
-        parseNamedAttributeDecl(llvm::None);
-    if (failed(decl))
-      return failure();
-
-    ast::NamedAttributeDecl *namedDecl = *decl;
-
-    std::string stringAttrValue =
-        "\"" + std::string((*namedDecl).getName().getName()) + "\"";
-    auto *stringAttr = ast::AttributeExpr::create(ctx, loc, stringAttrValue);
-
-    // Declare it as a variable
-    std::string anonName =
-        llvm::formatv("dict{0}", anonymousDeclNameCounter++).str();
-    FailureOr<ast::VariableDecl *> stringAttrDecl =
-        createVariableDecl(anonName, namedDecl->getLoc(), stringAttr, {});
-    if (failed(stringAttrDecl))
-      return failure();
-
-    // Get its reference
-    auto stringAttrRef = parseDeclRefExpr(
-        (*stringAttrDecl)->getName().getName(), namedDecl->getLoc());
-    if (failed(stringAttrRef))
-      return failure();
-
-    // Create addEntryToDictionaryAttr native call.
-    SmallVector<ast::Expr *> arrayAttrArgs{*dictAttrCall, *stringAttrRef,
-                                           namedDecl->getValue()};
-    auto entryToDictionaryCall =
-        createNativeCall(loc, "addEntryToDictionaryAttr", arrayAttrArgs);
-    if (failed(entryToDictionaryCall))
-      return failure();
-
-    // Uses the new array for the next element.
-    dictAttrCall = entryToDictionaryCall;
-  } while (consumeIf(Token::comma));
-  if (failed(parseToken(Token::r_brace,
-                        "expected `}` to close dictionary attribute")))
-    return failure();
-  return dictAttrCall;
-}
-
-FailureOr<ast::Expr *> Parser::parseArrayAttrExpr() {
-
-  // Advance to the next token without failing.
-  auto nextToken = [&](Token &curToken, int64_t offset) {
-    SMRange loc = curToken.getLoc();
-    SMRange dictArraysLoc(
-        loc.Start.getFromPointer(loc.Start.getPointer() + offset),
-        curToken.getEndLoc());
-    resetToken(dictArraysLoc);
-  };
-
-  SMRange loc = curToken.getLoc();
-
-  const char tokenAfterArray = *loc.End.getPointer();
-  if (tokenAfterArray != '[')
-    return emitError(curToken.getLoc(), "expected `[` after `array`.");
-
-  // Consume `array[` token by advancing 6 characters.
-  // Since the lexer misinterprets `[{` as a string_block, we can't consume the
-  // array token in the normal way. Instead, advance to the next token without
-  // looking at the new Token::Kind.
-  nextToken(curToken, 6);
-
-  if (parserContext != ParserContext::Rewrite)
-    return emitError(
-        "Parsing of array attributes as constraint not supported!");
-
-  auto arrayAttrCall = createNativeCall(loc, "createArrayAttr", {});
-  if (failed(arrayAttrCall))
-    return failure();
-
-  do {
-    FailureOr<ast::Expr *> attr = parseExpr();
-    if (failed(attr))
-      return failure();
-
-    SmallVector<ast::Expr *> arrayAttrArgs{*arrayAttrCall, *attr};
-    auto elemToArrayCall =
-        createNativeCall(loc, "addElemToArrayAttr", arrayAttrArgs);
-    if (failed(elemToArrayCall))
-      return failure();
-
-    // Uses the new array for the next element.
-    arrayAttrCall = elemToArrayCall;
-  } while (consumeIf(Token::comma));
-
-  if (failed(
-          parseToken(Token::r_square, "expected `]` to close array attribute")))
-    return failure();
-  return arrayAttrCall;
-}
-
 //===----------------------------------------------------------------------===//
 // Stmts
 
@@ -2887,6 +2876,35 @@ Parser::createMemberAccessExpr(ast::Expr *parentExpr, StringRef name,
   return ast::MemberAccessExpr::create(ctx, loc, parentExpr, name, *memberType);
 }
 
+FailureOr<ast::DeclRefExpr *>
+Parser::createNativeCall(SMRange loc, StringRef nativeFuncName,
+                         MutableArrayRef<ast::Expr *> arguments) {
+
+  FailureOr<ast::Expr *> nativeFuncExpr = parseDeclRefExpr(nativeFuncName, loc);
+  if (failed(nativeFuncExpr))
+    return emitError(nativeFuncName + " not found.");
+
+  if (!(*nativeFuncExpr)->getType().isa<ast::RewriteType>())
+    return emitError(nativeFuncName + " should be defined as a rewriter.");
+
+  FailureOr<ast::CallExpr *> nativeCall =
+      createCallExpr(loc, *nativeFuncExpr, arguments);
+  if (failed(nativeCall))
+    return failure();
+
+  // Create a unique anonymous name to use, as the name for this decl is not
+  // important.
+  std::string anonName =
+      llvm::formatv("{0}_{1}", nativeFuncName, anonymousDeclNameCounter++)
+          .str();
+  FailureOr<ast::VariableDecl *> varDecl = defineVariableDecl(
+      anonName, loc, (*nativeCall)->getType(), *nativeCall, {});
+  if (failed(varDecl))
+    return failure();
+
+  return createDeclRefExpr(loc, *varDecl);
+}
+
 FailureOr<ast::Type> Parser::validateMemberAccess(ast::Expr *parentExpr,
                                                   StringRef name, SMRange loc) {
   ast::Type parentType = parentExpr->getType();
@@ -3166,38 +3184,6 @@ Parser::createTupleExpr(SMRange loc, ArrayRef<ast::Expr *> elements,
   return ast::TupleExpr::create(ctx, loc, elements, elementNames);
 }
 
-FailureOr<ast::DeclRefExpr *>
-Parser::createNativeCall(SMRange loc, StringRef nativeFuncName,
-                         MutableArrayRef<ast::Expr *> arguments) {
-
-  FailureOr<ast::Expr *> nativeFuncExpr = parseDeclRefExpr(nativeFuncName, loc);
-  if (failed(nativeFuncExpr))
-    return emitError(nativeFuncName + " not found.");
-
-  if (!(*nativeFuncExpr)->getType().isa<ast::RewriteType>())
-    return emitError(nativeFuncName + " should be defined as a rewriter.");
-
-  FailureOr<ast::CallExpr *> nativeCall =
-      createCallExpr(loc, *nativeFuncExpr, arguments);
-  if (failed(nativeCall))
-    return failure();
-
-  // Create a unique anonymous name to use, as the name for this decl is not
-  // important.
-  std::string anonName =
-      llvm::formatv("{0}_{1}", nativeFuncName, anonymousDeclNameCounter++)
-          .str();
-  FailureOr<ast::VariableDecl *> varDecl = defineVariableDecl(
-      anonName, loc, (*nativeCall)->getType(), *nativeCall, {});
-  if (failed(varDecl))
-    return failure();
-
-  FailureOr<ast::DeclRefExpr *> arrayAttrReference =
-      createDeclRefExpr(loc, *varDecl);
-
-  return *arrayAttrReference;
-}
-
 //===----------------------------------------------------------------------===//
 // Stmts