Enable useDefault{Type/Attribute}PrinterParser by default in ODS Dialect definition

[mehdi_amini]

Summary

The majority of dialects reimplement the same boilerplate over and over,
switching the default makes it for better discoverability and make it simpler
to implement new dialects.

[rriddle]

mlir/lib/IR/BuiltinAttributes.cpp

14 ↗

(On Diff #400672)

Are these necessary?

mlir/lib/IR/Location.cpp

11 ↗

(On Diff #400672)

What is this used for?

[mehdi_amini]

Remove unnessary includes

[mehdi_amini]

mlir/lib/IR/Location.cpp

11 ↗

(On Diff #400672)

It was needed originally before I disabled it in the .td file for this dialect.

The generated printer/parser uses some AsmParser defined there.

[jpienaar]

switching the default makes it for better discoverability

So too would making it unset by default (?) and flagging a warning at build time asking it to be explicitly set either way and explaining benefit of having it on (so no warning if 0). That is not a behavior change but pushes people in a better direction. Or it could simply be used to give folks a heads-up before flipping. Like a PSA but in code that they can see in their presubmit.

The dialects changed here can be done in separate changes from the default flips.

[mehdi_amini]

committing the dialect changes without changing the default would require to change the setting on each dialect individually back and forth, so I'd rather avoid the unnecessary churn here.

[mehdi_amini]

Tweak the ODS generators logic to have the decl/def generators in sync in their logic to decide when to emit or not the print/parse Type/Attribute methods.

[rriddle]

mlir/lib/IR/BuiltinAttributes.cpp

14 ↗

(On Diff #400672)

Unresolved?

mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp

642

[mehdi_amini]

address comments

mlir/lib/IR/BuiltinAttributes.cpp

14 ↗

(On Diff #400672)

Strange, I thought I removed these...

mlir/include/mlir/IR/BuiltinDialect.td

 #ifndef BUILTIN_BASE
 #define BUILTIN_BASE
 
 include "mlir/IR/OpBase.td"
 
 def Builtin_Dialect : Dialect {
   let summary =
     "A dialect containing the builtin Attributes, Operations, and Types";
-
   let name = "builtin";
   let cppNamespace = "::mlir";
+  let useDefaultAttributePrinterParser = 0;
+  let useDefaultTypePrinterParser = 0;
   let extraClassDeclaration = [{
   private:
     // Register the builtin Attributes.
     void registerAttributes();
     // Register the builtin Location Attributes.
     void registerLocationAttributes();
     // Register the builtin Types.
     void registerTypes();
 
   public:
   }];
 }
 
 #endif // BUILTIN_BASE

mlir/include/mlir/IR/OpBase.td

class Dialect {
   // If this dialect overrides the hook for verifying region result attributes.
   bit hasRegionResultAttrVerify = 0;
 
   // If this dialect overrides the hook for op interface fallback.
   bit hasOperationInterfaceFallback = 0;
 
   // If this dialect should use default generated attribute parser boilerplate:
   // it'll dispatch the parsing to every individual attributes directly.
-  bit useDefaultAttributePrinterParser = 0;
+  bit useDefaultAttributePrinterParser = 1;
 
   // If this dialect should use default generated type parser boilerplate:
   // it'll dispatch the parsing to every individual types directly.
-  bit useDefaultTypePrinterParser = 0;
+  bit useDefaultTypePrinterParser = 1;
 
   // If this dialect overrides the hook for canonicalization patterns.
   bit hasCanonicalizer = 0;
 
   // Whether to emit raw/with no prefix or format changes, or emit with
   // accessor with prefix only and UpperCamel suffix or to emit accessors with
   // both.
   //

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

 Type ValueType::parse(mlir::AsmParser &parser) {
   Type ty;
   if (parser.parseLess() || parser.parseType(ty) || parser.parseGreater()) {
     parser.emitError(parser.getNameLoc(), "failed to parse async value type");
     return Type();
   }
   return ValueType::get(ty);
 }
-
-/// 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 typeTag;
-  if (parser.parseKeyword(&typeTag))
-    return Type();
-  Type genType;
-  auto parseResult = generatedTypeParser(parser, typeTag, genType);
-  if (parseResult.hasValue())
-    return genType;
-  parser.emitError(parser.getNameLoc(), "unknown async type: ") << typeTag;
-  return {};
-}

mlir/lib/Dialect/EmitC/IR/EmitC.cpp

if (parser.parseOptionalString(&value)) {
     parser.emitError(loc) << "expected string";
     return Attribute();
   }
   if (parser.parseGreater())
     return Attribute();
   return get(parser.getContext(), value);
 }
 
-Attribute EmitCDialect::parseAttribute(DialectAsmParser &parser,
-                                       Type type) const {
-  llvm::SMLoc typeLoc = parser.getCurrentLocation();
-  StringRef mnemonic;
-  if (parser.parseKeyword(&mnemonic))
-    return Attribute();
-  Attribute genAttr;
-  OptionalParseResult parseResult =
-      generatedAttributeParser(parser, mnemonic, type, genAttr);
-  if (parseResult.hasValue())
-    return genAttr;
-  parser.emitError(typeLoc, "unknown attribute in EmitC dialect");
-  return Attribute();
-}
-
-void EmitCDialect::printAttribute(Attribute attr, DialectAsmPrinter &os) const {
-  if (failed(generatedAttributePrinter(attr, os)))
-    llvm_unreachable("unexpected 'EmitC' attribute kind");
-}
-
 void emitc::OpaqueAttr::print(AsmPrinter &printer) const {
   printer << "<\"";
   llvm::printEscapedString(getValue(), printer.getStream());
   printer << "\">";
 }
 
 //===----------------------------------------------------------------------===//
 // EmitC Types

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

do {
     options.push_back(std::make_pair(*option, value));
   } while (succeeded(parser.parseOptionalComma()));
   if (failed(parser.parseGreater()))
     return {};
 
   llvm::sort(options, llvm::less_first());
   return get(parser.getContext(), options);
 }
-
-Attribute LLVMDialect::parseAttribute(DialectAsmParser &parser,
-                                      Type type) const {
-  if (type) {
-    parser.emitError(parser.getNameLoc(), "unexpected type");
-    return {};
-  }
-  StringRef attrKind;
-  if (parser.parseKeyword(&attrKind))
-    return {};
-  {
-    Attribute attr;
-    auto parseResult = generatedAttributeParser(parser, attrKind, type, attr);
-    if (parseResult.hasValue())
-      return attr;
-  }
-  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

static Type parsePDLType(AsmParser &parser) {
   // 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: `")
       << typeTag << "'";
   return Type();
 }
 
-Type PDLDialect::parseType(DialectAsmParser &parser) const {
-  return parsePDLType(parser);
-}
-
-void PDLDialect::printType(Type type, DialectAsmPrinter &printer) const {
-  if (failed(generatedTypePrinter(type, printer)))
-    llvm_unreachable("unknown 'pdl' type");
-}
-
 //===----------------------------------------------------------------------===//
 // PDL Types
 //===----------------------------------------------------------------------===//
 
 bool PDLType::classof(Type type) {
   return llvm::isa<PDLDialect>(type.getDialect());
 }
 

mlir/lib/Dialect/SparseTensor/IR/SparseTensorDialect.cpp

#include "mlir/Dialect/SparseTensor/IR/SparseTensorAttrDefs.cpp.inc"
   addOperations<
 #define GET_OP_LIST
 #include "mlir/Dialect/SparseTensor/IR/SparseTensorOps.cpp.inc"
       >();
 }
 
 #define GET_OP_CLASSES
 #include "mlir/Dialect/SparseTensor/IR/SparseTensorOps.cpp.inc"
-
-Attribute SparseTensorDialect::parseAttribute(DialectAsmParser &parser,
-                                              Type type) const {
-  StringRef attrTag;
-  if (failed(parser.parseKeyword(&attrTag)))
-    return Attribute();
-  Attribute attr;
-  auto parseResult = generatedAttributeParser(parser, attrTag, type, attr);
-  if (parseResult.hasValue())
-    return attr;
-  parser.emitError(parser.getNameLoc(), "unknown sparse tensor attribute");
-  return Attribute();
-}
-
-void SparseTensorDialect::printAttribute(Attribute attr,
-                                         DialectAsmPrinter &printer) const {
-  if (succeeded(generatedAttributePrinter(attr, printer)))
-    return;
-}

mlir/test/lib/Dialect/Test/TestDialect.td

def Test_Dialect : Dialect {
   let emitAccessorPrefix = kEmitAccessorPrefix_Prefixed;
   let hasCanonicalizer = 1;
   let hasConstantMaterializer = 1;
   let hasOperationAttrVerify = 1;
   let hasRegionArgAttrVerify = 1;
   let hasRegionResultAttrVerify = 1;
   let hasOperationInterfaceFallback = 1;
   let hasNonDefaultDestructor = 1;
-  let useDefaultAttributePrinterParser = 1;
+  let useDefaultTypePrinterParser = 0;
   let dependentDialects = ["::mlir::DLTIDialect"];
 
   let extraClassDeclaration = [{
     void registerAttributes();
     void registerTypes();
 
     // Provides a custom printing/parsing for some operations.
     ::llvm::Optional<ParseOpHook>
       getParseOperationHook(::llvm::StringRef opName) const override;
     ::llvm::unique_function<void(::mlir::Operation *,
                                  ::mlir::OpAsmPrinter &printer)>
      getOperationPrinter(::mlir::Operation *op) const override;
 
+     ::mlir::Type parseType(::mlir::DialectAsmParser &parser) const override;
+     void printType(::mlir::Type type,
+                    ::mlir::DialectAsmPrinter &printer) const override;
   private:
     // Storage for a custom fallback interface.
     void *fallbackEffectOpInterfaces;
 
   }];
 }
 
 #endif // TEST_DIALECT

mlir/test/mlir-tblgen/attr-or-type-format.td

 // RUN: mlir-tblgen -gen-attrdef-defs -I %S/../../include %s | FileCheck %s --check-prefix=ATTR
 // RUN: mlir-tblgen -gen-typedef-defs -I %S/../../include %s | FileCheck %s --check-prefix=TYPE
 
 include "mlir/IR/OpBase.td"
 
 /// Test that attribute and type printers and parsers are correctly generated.
 def Test_Dialect : Dialect {
   let name = "TestDialect";
   let cppNamespace = "::test";
+  let useDefaultTypePrinterParser = 0;
 }
 
 class TestAttr<string name> : AttrDef<Test_Dialect, name>;
 class TestType<string name> : TypeDef<Test_Dialect, name>;
 
 def AttrParamA : AttrParameter<"TestParamA", "an attribute param A"> {
   let parser = "::parseAttrParamA($_parser, $_type)";
   let printer = "::printAttrParamA($_printer, $_self)";

mlir/test/mlir-tblgen/typedefs.td

 // DEF-NEXT: }
 // DEF-NEXT: if (mnemonic == ::test::IndexType::getMnemonic()) {
 // DEF-NEXT:   value = ::test::IndexType::parse(parser);
 // DEF-NEXT:   return ::mlir::success(!!value);
 // DEF: return {};
 
 def Test_Dialect: Dialect {
 // DECL-NOT: TestDialect
-// DEF-NOT: TestDialect
     let name = "TestDialect";
     let cppNamespace = "::test";
 }
 
 class TestType<string name> : TypeDef<Test_Dialect, name> { }
 
 def A_SimpleTypeA : TestType<"SimpleA"> {
 // DECL: class SimpleAType : public ::mlir::Type

mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp

 /// This struct is the base generator used when processing tablegen interfaces.
 class DefGenerator {
 public:
   bool emitDecls(StringRef selectedDialect);
   bool emitDefs(StringRef selectedDialect);
 
 protected:
   DefGenerator(std::vector<llvm::Record *> &&defs, raw_ostream &os,
-               StringRef defType, StringRef valueType, bool isAttrGenerator)
+               StringRef defType, StringRef valueType, bool isAttrGenerator,
+               bool needsDialectParserPrinter)
       : defRecords(std::move(defs)), os(os), defType(defType),
-        valueType(valueType), isAttrGenerator(isAttrGenerator) {}
+        valueType(valueType), isAttrGenerator(isAttrGenerator),
+        needsDialectParserPrinter(needsDialectParserPrinter) {}
 
   /// Emit the list of def type names.
   void emitTypeDefList(ArrayRef<AttrOrTypeDef> defs);
   /// Emit the code to dispatch between different defs during parsing/printing.
   void emitParsePrintDispatch(ArrayRef<AttrOrTypeDef> defs);
 
   /// The set of def records to emit.
   std::vector<llvm::Record *> defRecords;
   /// The attribute or type class to emit.
   /// The stream to emit to.
   raw_ostream &os;
   /// The prefix of the tablegen def name, e.g. Attr or Type.
   StringRef defType;
   /// The C++ base value type of the def, e.g. Attribute or Type.
   StringRef valueType;
   /// Flag indicating if this generator is for Attributes. False if the
   /// generator is for types.
   bool isAttrGenerator;
+  /// Track if we need to emit the printAttribute/parseAttribute
+  /// implementations.
+  bool needsDialectParserPrinter;
 };
 
 /// A specialized generator for AttrDefs.
 struct AttrDefGenerator : public DefGenerator {
   AttrDefGenerator(const llvm::RecordKeeper &records, raw_ostream &os)
       : DefGenerator(records.getAllDerivedDefinitions("AttrDef"), os, "Attr",
-                     "Attribute", /*isAttrGenerator=*/true) {}
+                     "Attribute",
+                     /*isAttrGenerator=*/true,
+                     /*needsDialectParserPrinter=*/
+                     !records.getAllDerivedDefinitions("DialectAttr").empty()) {
+  }
 };
 /// A specialized generator for TypeDefs.
 struct TypeDefGenerator : public DefGenerator {
   TypeDefGenerator(const llvm::RecordKeeper &records, raw_ostream &os)
       : DefGenerator(records.getAllDerivedDefinitions("TypeDef"), os, "Type",
-                     "Type", /*isAttrGenerator=*/false) {}
+                     "Type", /*isAttrGenerator=*/false,
+                     /*needsDialectParserPrinter=*/
+                     !records.getAllDerivedDefinitions("DialectType").empty()) {
+  }
 };
 } // namespace
 
 //===----------------------------------------------------------------------===//
 // GEN: Declarations
 //===----------------------------------------------------------------------===//
 
 /// Print this above all the other declarations. Contains type declarations used
for (const AttrOrTypeDef &def : defs) {
     if (!def.getDialect().getCppNamespace().empty())
       os << "DEFINE_EXPLICIT_TYPE_ID(" << def.getDialect().getCppNamespace()
          << "::" << def.getCppClassName() << ")\n";
   }
 
   Dialect firstDialect = defs.front().getDialect();
   // Emit the default parser/printer for Attributes if the dialect asked for
   // it.
-  if (valueType == "Attribute" &&
+  if (valueType == "Attribute" && needsDialectParserPrinter &&
       firstDialect.useDefaultAttributePrinterParser()) {
     NamespaceEmitter nsEmitter(os, firstDialect);
     os << llvm::formatv(dialectDefaultAttrPrinterParserDispatch,
                         firstDialect.getCppClassName());
   }
 
   // Emit the default parser/printer for Types if the dialect asked for it.
-  if (valueType == "Type" && firstDialect.useDefaultTypePrinterParser()) {
+  if (valueType == "Type" && needsDialectParserPrinter &&
+      firstDialect.useDefaultTypePrinterParser()) {
     NamespaceEmitter nsEmitter(os, firstDialect);
     os << llvm::formatv(dialectDefaultTypePrinterParserDispatch,
                         firstDialect.getCppClassName());
   }
 
   return false;
 }
 

mlir/tools/mlir-tblgen/DialectGen.cpp

// Emit all nested namespaces.
 
     // Emit the start of the decl.
     std::string cppName = dialect.getCppClassName();
     os << llvm::formatv(dialectDeclBeginStr, cppName, dialect.getName(),
                         dependentDialectRegistrations);
 
     // Check for any attributes/types registered to this dialect.  If there are,
     // add the hooks for parsing/printing.
-    if (!dialectAttrs.empty() || dialect.useDefaultAttributePrinterParser())
+    if (!dialectAttrs.empty() && dialect.useDefaultAttributePrinterParser())
       os << attrParserDecl;
-    if (!dialectTypes.empty() || dialect.useDefaultTypePrinterParser())
+    if (!dialectTypes.empty() && dialect.useDefaultTypePrinterParser())
       os << typeParserDecl;
 
     // Add the decls for the various features of the dialect.
     if (dialect.hasCanonicalizer())
       os << canonicalizerDecl;
     if (dialect.hasConstantMaterializer())
       os << constantMaterializerDecl;
     if (dialect.hasOperationAttrVerify())