Switch generatedTypeParser/generatedAttributeParser to return an OptionalParseResult

[mehdi_amini]

Summary

This allows the caller to distinguish between a parse error or an
unmatched keyword. It fixes the redundant error that was emitted by the
caller when the generated parser would fail.

[ftynse]

mlir/lib/Dialect/Async/IR/Async.cpp

337–344

Nit: the extra scope is not necessary at the end of the function

[jpienaar]

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp

2510–2517

Could you fix this error style while here?

[rriddle]

mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp

816

Missing newlines at the end of this line and the one below.

[mehdi_amini]

Address comments

[schweitz]

Broke flang builds.

[mehdi_amini]

Sorry about that, I have a local fix, still running the tests and I'll push shortly!

mlir/lib/Dialect/ArmSVE/IR/ArmSVEDialect.cpp

 #include "mlir/Dialect/ArmSVE/ArmSVETypes.cpp.inc"
 
 //===----------------------------------------------------------------------===//
 // ScalableVectorType
 //===----------------------------------------------------------------------===//
 
 Type arm_sve::ArmSVEDialect::parseType(DialectAsmParser &parser) const {
   llvm::SMLoc typeLoc = parser.getCurrentLocation();
-  auto genType = generatedTypeParser(getContext(), parser, "vector");
-  if (genType != Type())
+  {
+    Type genType;
+    auto parseResult = generatedTypeParser(parser.getBuilder().getContext(),
+                                           parser, "vector", genType);
+    if (parseResult.hasValue())
       return genType;
+  }
   parser.emitError(typeLoc, "unknown type in ArmSVE dialect");
   return Type();
 }
 
 void arm_sve::ArmSVEDialect::printType(Type type, DialectAsmPrinter &os) const {
   if (failed(generatedTypePrinter(type, os)))
     llvm_unreachable("unexpected 'arm_sve' type kind");
 }

mlir/lib/Dialect/Async/IR/Async.cpp

 /// Print a type registered to this dialect.
 void AsyncDialect::printType(Type type, DialectAsmPrinter &os) const {
   if (failed(generatedTypePrinter(type, os)))
     llvm_unreachable("unexpected 'async' type kind");
 }
 
 /// Parse a type registered to this dialect.
 Type AsyncDialect::parseType(DialectAsmParser &parser) const {
-  StringRef mnemonic;
-  if (parser.parseKeyword(&mnemonic))
+  StringRef typeTag;
+  if (parser.parseKeyword(&typeTag))
     return Type();
-
-  return generatedTypeParser(getContext(), parser, mnemonic);
+  Type genType;
+  auto parseResult = generatedTypeParser(parser.getBuilder().getContext(),
+                                         parser, typeTag, genType);
+  if (parseResult.hasValue())
+    return genType;
+  parser.emitError(parser.getNameLoc(), "unknown async attribute type: ")
+      << attrKind;
+  return {};
 }

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp

Attribute LLVMDialect::parseAttribute(DialectAsmParser &parser,
                                       Type type) const {
   if (type) {
     parser.emitError(parser.getNameLoc(), "unexpected type");
     return {};
   }
   StringRef attrKind;
   if (parser.parseKeyword(&attrKind))
     return {};
-  if (auto attr =
-          generatedAttributeParser(getContext(), parser, attrKind, type))
+  {
+    Attribute attr;
+    auto parseResult =
+        generatedAttributeParser(getContext(), parser, attrKind, type, attr);
+    if (parseResult.hasValue())
       return attr;
-
-  parser.emitError(parser.getNameLoc(), "Unknown attribute type: ") << attrKind;
+  }
+  parser.emitError(parser.getNameLoc(), "unknown attribute type: ") << attrKind;
   return {};
 }
 
 void LLVMDialect::printAttribute(Attribute attr, DialectAsmPrinter &os) const {
   if (succeeded(generatedAttributePrinter(attr, os)))
     return;
   llvm_unreachable("Unknown attribute type");
 }

mlir/lib/Dialect/PDL/IR/PDLTypes.cpp

 #define GET_TYPEDEF_CLASSES
 #include "mlir/Dialect/PDL/IR/PDLOpsTypes.cpp.inc"
 
 //===----------------------------------------------------------------------===//
 // PDLDialect
 //===----------------------------------------------------------------------===//
 
 static Type parsePDLType(DialectAsmParser &parser) {
-  StringRef keyword;
-  if (parser.parseKeyword(&keyword))
+  StringRef typeTag;
+  if (parser.parseKeyword(&typeTag))
     return Type();
-  if (Type type = generatedTypeParser(parser.getBuilder().getContext(), parser,
-                                      keyword))
-    return type;
+  {
+    Type genType;
+    auto parseResult = generatedTypeParser(parser.getBuilder().getContext(),
+                                           parser, typeTag, genType);
+    if (parseResult.hasValue())
+      return genType;
+  }
 
   // FIXME: This ends up with a double error being emitted if `RangeType` also
   // emits an error. We should rework the `generatedTypeParser` to better
   // support when the keyword is valid but the individual type parser itself
   // emits an error.
   parser.emitError(parser.getNameLoc(), "invalid 'pdl' type: `")
-      << keyword << "'";
+      << typeTag << "'";
   return Type();
 }
 
 Type PDLDialect::parseType(DialectAsmParser &parser) const {
   return parsePDLType(parser);
 }
 
 void PDLDialect::printType(Type type, DialectAsmPrinter &printer) const {

mlir/test/Dialect/LLVMIR/invalid.mlir

^bb4:
       llvm.return
   }
 }
 
 // -----
 
 module {
   llvm.func @loopOptions() {
-      // expected-error@+2 {{unknown loop option: name}}
-      // expected-error@below {{Unknown attribute type: loopopts}}
+      // expected-error@below {{unknown loop option: name}}
       llvm.br ^bb4 {llvm.loop = {options = #llvm.loopopts<name>}}
     ^bb4:
       llvm.return
   }
 }
 
 // -----
 
 module {
   llvm.func @loopOptions() {
-      // expected-error@+2 {{loop option present twice}}
-      // expected-error@below {{Unknown attribute type: loopopts}}
+      // expected-error@below {{loop option present twice}}
       llvm.br ^bb4 {llvm.loop = {options = #llvm.loopopts<disable_licm = true, disable_licm = true>}}
     ^bb4:
       llvm.return
   }
 }
 
 // -----
 

mlir/test/Dialect/PDL/invalid-types.mlir

 // RUN: mlir-opt %s -split-input-file -verify-diagnostics
 
 //===----------------------------------------------------------------------===//
 // pdl::RangeType
 //===----------------------------------------------------------------------===//
 
-// expected-error@+2 {{element of pdl.range cannot be another range, but got'!pdl.range<value>'}}
-// expected-error@+1 {{invalid 'pdl' type}}
+// expected-error@below {{element of pdl.range cannot be another range, but got'!pdl.range<value>'}}
 #invalid_element = !pdl.range<range<value>>

mlir/test/lib/Dialect/Test/TestAttributes.cpp

 // TestDialect
 //===----------------------------------------------------------------------===//
 
 Attribute TestDialect::parseAttribute(DialectAsmParser &parser,
                                       Type type) const {
   StringRef attrTag;
   if (failed(parser.parseKeyword(&attrTag)))
     return Attribute();
-  if (auto attr = generatedAttributeParser(getContext(), parser, attrTag, type))
+  {
+    Attribute attr;
+    auto parseResult =
+        generatedAttributeParser(getContext(), parser, attrTag, type, attr);
+    if (parseResult.hasValue())
       return attr;
-
+  }
   parser.emitError(parser.getNameLoc(), "unknown test attribute");
   return Attribute();
 }
 
 void TestDialect::printAttribute(Attribute attr,
                                  DialectAsmPrinter &printer) const {
   if (succeeded(generatedAttributePrinter(attr, printer)))
     return;
 }

mlir/test/lib/Dialect/Test/TestTypes.cpp

 //===----------------------------------------------------------------------===//
 
 static Type parseTestType(MLIRContext *ctxt, DialectAsmParser &parser,
                           llvm::SetVector<Type> &stack) {
   StringRef typeTag;
   if (failed(parser.parseKeyword(&typeTag)))
     return Type();
 
-  auto genType = generatedTypeParser(ctxt, parser, typeTag);
-  if (genType != Type())
+  {
+    Type genType;
+    auto parseResult = generatedTypeParser(ctxt, parser, typeTag, genType);
+    if (parseResult.hasValue())
       return genType;
-
+  }
   if (typeTag == "test_type")
     return TestType::get(parser.getBuilder().getContext());
 
   if (typeTag != "test_rec")
     return Type();
 
   StringRef name;
   if (parser.parseLess() || parser.parseKeyword(&name))

mlir/test/mlir-tblgen/attrdefs.td

 
 // DEF: #ifdef GET_ATTRDEF_LIST
 // DEF: #undef GET_ATTRDEF_LIST
 // DEF: ::mlir::test::SimpleAAttr,
 // DEF: ::mlir::test::CompoundAAttr,
 // DEF: ::mlir::test::IndexAttr,
 // DEF: ::mlir::test::SingleParameterAttr
 
-// DEF-LABEL: ::mlir::Attribute generatedAttributeParser(::mlir::MLIRContext *context,
+// DEF-LABEL: OptionalParseResult generatedAttributeParser(::mlir::MLIRContext *context,
 // DEF-NEXT: ::mlir::DialectAsmParser &parser,
-// DEF-NEXT: ::llvm::StringRef mnemonic, ::mlir::Type type) {
-// DEF: if (mnemonic == ::mlir::test::CompoundAAttr::getMnemonic()) return ::mlir::test::CompoundAAttr::parse(context, parser, type);
-// DEF-NEXT: if (mnemonic == ::mlir::test::IndexAttr::getMnemonic()) return ::mlir::test::IndexAttr::parse(context, parser, type);
-// DEF: return ::mlir::Attribute();
+// DEF-NEXT: ::llvm::StringRef mnemonic, ::mlir::Type type,
+// DEF-NEXT: ::mlir::Attribute &value) {
+// DEF: if (mnemonic == ::mlir::test::CompoundAAttr::getMnemonic()) {
+// DEF-NEXT: value = ::mlir::test::CompoundAAttr::parse(context, parser, type);
+// DEF-NEXT: return success(!!value);
+// DEF-NEXT: } if (mnemonic == ::mlir::test::IndexAttr::getMnemonic()) {
+// DEF-NEXT:   value = ::mlir::test::IndexAttr::parse(context, parser, type);
+// DEF-NEXT:   return success(!!value);
+// DEF: return {};
 
 def Test_Dialect: Dialect {
 // DECL-NOT: TestDialect
 // DEF-NOT: TestDialect
     let name = "TestDialect";
     let cppNamespace = "::mlir::test";
 }
 

mlir/test/mlir-tblgen/typedefs.td

 // DEF: #ifdef GET_TYPEDEF_LIST
 // DEF: #undef GET_TYPEDEF_LIST
 // DEF: ::mlir::test::SimpleAType,
 // DEF: ::mlir::test::CompoundAType,
 // DEF: ::mlir::test::IndexType,
 // DEF: ::mlir::test::SingleParameterType,
 // DEF: ::mlir::test::IntegerType
 
-// DEF-LABEL: ::mlir::Type generatedTypeParser(::mlir::MLIRContext *context,
+// DEF-LABEL: OptionalParseResult generatedTypeParser(::mlir::MLIRContext *context,
 // DEF-NEXT: ::mlir::DialectAsmParser &parser,
-// DEF-NEXT: ::llvm::StringRef mnemonic) {
-// DEF: if (mnemonic == ::mlir::test::CompoundAType::getMnemonic()) return ::mlir::test::CompoundAType::parse(context, parser);
-// DEF: return ::mlir::Type();
+// DEF-NEXT: ::llvm::StringRef mnemonic,
+// DEF-NEXT: ::mlir::Type &value) {
+// DEF: if (mnemonic == ::mlir::test::CompoundAType::getMnemonic()) {
+// DEF-NEXT:   value = ::mlir::test::CompoundAType::parse(context, parser);
+// DEF-NEXT:   return success(!!value);
+// DEF-NEXT: } if (mnemonic == ::mlir::test::IndexType::getMnemonic()) {
+// DEF-NEXT:   value = ::mlir::test::IndexType::parse(context, parser);
+// DEF-NEXT:   return success(!!value);
+// DEF: return {};
 
 def Test_Dialect: Dialect {
 // DECL-NOT: TestDialect
 // DEF-NOT: TestDialect
     let name = "TestDialect";
     let cppNamespace = "::mlir::test";
 }
 

mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp

 // GEN: Definitions
 //===----------------------------------------------------------------------===//
 
 /// The code block used to start the auto-generated parser function.
 ///
 /// {0}: The name of the base value type, e.g. Attribute or Type.
 /// {1}: Additional parser parameters.
 static const char *const defParserDispatchStartStr = R"(
-static ::mlir::{0} generated{0}Parser(::mlir::MLIRContext *context,
+static OptionalParseResult generated{0}Parser(::mlir::MLIRContext *context,
                                       ::mlir::DialectAsmParser &parser,
-                                      ::llvm::StringRef mnemonic{1}) {{
+                                      ::llvm::StringRef mnemonic{1},
+                                      ::mlir::{0} &value) {{
 )";
 
 /// The code block used to start the auto-generated printer function.
 ///
 /// {0}: The name of the base value type, e.g. Attribute or Type.
 static const char *const defPrinterDispatchStartStr = R"(
 static ::mlir::LogicalResult generated{0}Printer(
                          ::mlir::{0} def, ::mlir::DialectAsmPrinter &printer) {{
void DefGenerator::emitParsePrintDispatch(ArrayRef<AttrOrTypeDef> defs) {
   }
 
   // The parser dispatch is just a list of if-elses, matching on the mnemonic
   // and calling the def's parse function.
   os << llvm::formatv(defParserDispatchStartStr, valueType,
                       isAttrGenerator ? ", ::mlir::Type type" : "");
   for (const AttrOrTypeDef &def : defs) {
     if (def.getMnemonic()) {
-      os << formatv(
-          "  if (mnemonic == {0}::{1}::getMnemonic()) return {0}::{1}::",
+      os << formatv("  if (mnemonic == {0}::{1}::getMnemonic()) { \n"
+                    "    value = {0}::{1}::",
                     def.getDialect().getCppNamespace(), def.getCppClassName());
 
       // If the def has no parameters and no parser code, just invoke a normal
       // `get`.
       if (def.getNumParameters() == 0 && !def.getParserCode()) {
-        os << "get(context);\n";
+        os << "get(context);\n    return success(!!value);\n  }\n";
         continue;
       }
 
       os << "parse(context, parser" << (isAttrGenerator ? ", type" : "")
-         << ");\n";
+         << ");\n    return success(!!value);\n  }\n";
     }
   }
-  os << "  return ::mlir::" << valueType << "();\n";
+  os << "  return {};\n";
   os << "}\n\n";
 
   // The printer dispatch uses llvm::TypeSwitch to find and call the correct
   // printer.
   os << llvm::formatv(defPrinterDispatchStartStr, valueType);
   for (const AttrOrTypeDef &def : defs) {
     Optional<StringRef> mnemonic = def.getMnemonic();
     if (!mnemonic)