[mlir][mlir-tblgen] Emit correct error message if method is pruned (#160334)

Add verification for pruned methods for `emitCustomBuilder` and 
`emitCheckedCustomBuilder` with proper diagnostic about shadowed methods.

Without this verification, `mlir-tblgen` with `--gen-attrdef-decls`
would segmentation fault if custom builder is provided with its body,
but if method is pruned out due to duplication with other builders.

Fixes #160227 

---------

Co-authored-by: Justin Kim <[email protected]>

Author: JustinKim98

mlir/include/mlir/Dialect/Tosa/IR/TosaTypesBase.td

@@ -282,8 +282,7 @@ def Tosa_Shape : Tosa_Type<"shape", "shape"> {
      !tosa.shape<0>
     ```
   }];
-  let parameters = (ins "int" : $rank);
-  let builders = [TypeBuilder<(ins "int" : $rank)>];
+  let parameters = (ins "int":$rank);
   let assemblyFormat = "`<` $rank `>`";
 
   let genVerifyDecl = 1;

mlir/include/mlir/TableGen/Class.h

@@ -789,6 +789,10 @@ class Class {
         std::forward<Args>(args)...);
   }
 
+  const std::vector<std::unique_ptr<Method>> &getMethods() const {
+    return methods;
+  }
+
   /// Add a new field to the class. Class fields added this way are always
   /// private.
   template <typename TypeT, typename NameT>

mlir/test/mlir-tblgen/attr-duplicated-builder-error.td

@@ -0,0 +1,48 @@
+// RUN: not mlir-tblgen -gen-attrdef-decls -I %S/../../include %s 2>&1 | FileCheck %s
+
+include "mlir/IR/OpBase.td"
+
+def Test_Dialect : Dialect {
+  let name = "test";
+  let cppNamespace = "::test";
+}
+
+class TestAttr<string attrName, string attrMnemonic, list<Trait> traits = []>
+    : AttrDef<Test_Dialect, attrName, traits> {
+  let mnemonic = attrMnemonic;
+}
+
+def TestAttr : TestAttr<"Test", "test"> {
+  let summary = "Test attrubute";
+  let description = "Test attribute";
+
+  let parameters = (ins AttrParameter<"std::int64_t", "arg">:$arg);
+  let builders = [AttrBuilder<(ins "std::int64_t":$arg), [{
+            return $_get($_ctxt, arg);
+        }]>];
+
+  let assemblyFormat = "`<` $arg `>`";
+
+  let skipDefaultBuilders = 0;
+  let genVerifyDecl = 1;
+  let genMnemonicAlias = 1;
+}
+
+def Test_TestAttrOp : Op<Test_Dialect, "test", []> {
+  let summary = "test operation with attribute";
+  let description = "test operation with attribute";
+
+  let arguments = (ins TestAttr:$testAttr);
+  let assemblyFormat = "$testAttr attr-dict";
+}
+
+// CHECK: attr-duplicated-builder-error.td:20:7: error: builder `get` conflicts with an existing builder. 
+// CHECK-NEXT:   let builders = [AttrBuilder<(ins "std::int64_t":$arg), [{
+// CHECK-NEXT:       ^
+// CHECK-NEXT: note: A new builder with signature:
+// CHECK-NEXT: static TestAttr get(::mlir::MLIRContext *context, std::int64_t arg);
+// CHECK-EMPTY:
+// CHECK-NEXT: is shadowed by an existing builder with signature:
+// CHECK-NEXT: static TestAttr get(::mlir::MLIRContext *context, std::int64_t arg);
+// CHECK-EMPTY:
+// CHECK-NEXT: Please remove one of the conflicting definitions.

mlir/test/mlir-tblgen/attr-duplicated-custom-builders-error.td

@@ -0,0 +1,52 @@
+// RUN: not mlir-tblgen -gen-attrdef-decls -I %S/../../include %s 2>&1 | FileCheck %s
+
+include "mlir/IR/OpBase.td"
+
+def Test_Dialect : Dialect {
+  let name = "test";
+  let cppNamespace = "::test";
+}
+
+class TestAttr<string attrName, string attrMnemonic, list<Trait> traits = []>
+    : AttrDef<Test_Dialect, attrName, traits> {
+  let mnemonic = attrMnemonic;
+}
+
+def TestAttr : TestAttr<"Test", "test"> {
+  let summary = "Test attrubute";
+  let description = "Test attribute";
+
+  let parameters = (ins AttrParameter<"std::int64_t", "arg">:$arg);
+  let builders = [AttrBuilder<(ins "std::int64_t":$arg), [{
+            return $_get($_ctxt, arg);
+        }]>,
+                  AttrBuilder<(ins "std::int64_t":$arg), [{
+            // Duplicated builder
+            return $_get($_ctxt, arg);
+        }]>];
+
+  let assemblyFormat = "`<` $arg `>`";
+
+  let skipDefaultBuilders = 1;
+  let genVerifyDecl = 1;
+  let genMnemonicAlias = 1;
+}
+
+def Test_TestAttrOp : Op<Test_Dialect, "test", []> {
+  let summary = "test operation with attribute";
+  let description = "test operation with attribute";
+
+  let arguments = (ins TestAttr:$testAttr);
+  let assemblyFormat = "$testAttr attr-dict";
+}
+
+// CHECK: attr-duplicated-custom-builders-error.td:20:7: error: builder `get` conflicts with an existing builder.
+// CHECK-NEXT:   let builders = [AttrBuilder<(ins "std::int64_t":$arg), [{
+// CHECK-NEXT:   ^
+// CHECK-NEXT: note: A new builder with signature:
+// CHECK-NEXT: static TestAttr get(::mlir::MLIRContext *context, std::int64_t arg);
+// CHECK-EMPTY:
+// CHECK-NEXT: is shadowed by an existing builder with signature:
+// CHECK-NEXT: static TestAttr get(::mlir::MLIRContext *context, std::int64_t arg);
+// CHECK-EMPTY:
+// CHECK-NEXT: Please remove one of the conflicting definitions.

mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp

@@ -513,14 +513,57 @@ getCustomBuilderParams(std::initializer_list<MethodParameter> prefix,
   return builderParams;
 }
 
+static std::string getSignature(const Method &m) {
+  std::string signature;
+  llvm::raw_string_ostream os(signature);
+  raw_indented_ostream indentedOs(os);
+  m.writeDeclTo(indentedOs);
+  return signature;
+}
+
+static void emitDuplicatedBuilderError(const Method &currentMethod,
+                                       StringRef methodName,
+                                       const Class &defCls,
+                                       const AttrOrTypeDef &def) {
+
+  // Try to search for method that makes `get` redundant.
+  auto loc = def.getDef()->getFieldLoc("builders");
+  for (auto &method : defCls.getMethods()) {
+    if (method->getName() == methodName &&
+        method->makesRedundant(currentMethod)) {
+      PrintError(loc, llvm::Twine("builder `") + methodName +
+                          "` conflicts with an existing builder. ");
+      PrintFatalNote(llvm::Twine("A new builder with signature:\n") +
+                     getSignature(currentMethod) +
+                     "\nis shadowed by an existing builder with signature:\n" +
+                     getSignature(*method) +
+                     "\nPlease remove one of the conflicting "
+                     "definitions.");
+    }
+  }
+
+  // This code shouldn't be reached, but leaving this here for potential future
+  // use.
+  PrintFatalError(loc, "Failed to generate builder " + methodName);
+}
+
 void DefGen::emitCustomBuilder(const AttrOrTypeBuilder &builder) {
   // Don't emit a body if there isn't one.
   auto props = builder.getBody() ? Method::Static : Method::StaticDeclaration;
   StringRef returnType = def.getCppClassName();
   if (std::optional<StringRef> builderReturnType = builder.getReturnType())
     returnType = *builderReturnType;
-  Method *m = defCls.addMethod(returnType, "get", props,
-                               getCustomBuilderParams({}, builder));
+
+  llvm::StringRef methodName = "get";
+  const auto parameters = getCustomBuilderParams({}, builder);
+  Method *m = defCls.addMethod(returnType, methodName, props, parameters);
+
+  // If method is pruned, report error and terminate.
+  if (!m) {
+    auto curMethod = Method(returnType, methodName, props, parameters);
+    emitDuplicatedBuilderError(curMethod, methodName, defCls, def);
+  }
+
   if (!builder.getBody())
     return;
 
@@ -547,11 +590,19 @@ void DefGen::emitCheckedCustomBuilder(const AttrOrTypeBuilder &builder) {
   StringRef returnType = def.getCppClassName();
   if (std::optional<StringRef> builderReturnType = builder.getReturnType())
     returnType = *builderReturnType;
-  Method *m = defCls.addMethod(
-      returnType, "getChecked", props,
-      getCustomBuilderParams(
-          {{"::llvm::function_ref<::mlir::InFlightDiagnostic()>", "emitError"}},
-          builder));
+
+  llvm::StringRef methodName = "getChecked";
+  auto parameters = getCustomBuilderParams(
+      {{"::llvm::function_ref<::mlir::InFlightDiagnostic()>", "emitError"}},
+      builder);
+  Method *m = defCls.addMethod(returnType, methodName, props, parameters);
+
+  // If method is pruned, report error and terminate.
+  if (!m) {
+    auto curMethod = Method(returnType, methodName, props, parameters);
+    emitDuplicatedBuilderError(curMethod, methodName, defCls, def);
+  }
+
   if (!builder.getBody())
     return;
 

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

@@ -3104,8 +3104,8 @@ void OpEmitter::genBuilder() {
     std::optional<StringRef> body = builder.getBody();
     auto properties = body ? Method::Static : Method::StaticDeclaration;
     auto *method = opClass.addMethod("void", "build", properties, arguments);
-    if (body)
-      ERROR_IF_PRUNED(method, "build", op);
+
+    ERROR_IF_PRUNED(method, "build", op);
 
     if (method)
       method->setDeprecated(builder.getDeprecatedMessage());