From 0e28604f8687389ad2db09f83a029121d577e86f Mon Sep 17 00:00:00 2001 From: Justin Kim Date: Wed, 24 Sep 2025 00:57:59 +0900 Subject: [PATCH 1/4] [mlir][mlir-tblgen] emit correct error message if method is pruned --- .../mlir/Dialect/Tosa/IR/TosaTypesBase.td | 3 +-- mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp | 19 +++++++++++++++++++ 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/mlir/include/mlir/Dialect/Tosa/IR/TosaTypesBase.td b/mlir/include/mlir/Dialect/Tosa/IR/TosaTypesBase.td index 553d69cc21d17..93ab120339d55 100644 --- a/mlir/include/mlir/Dialect/Tosa/IR/TosaTypesBase.td +++ b/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; diff --git a/mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp b/mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp index 3140f12c0b7e8..b27e4cdcd5b38 100644 --- a/mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp +++ b/mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp @@ -513,6 +513,17 @@ getCustomBuilderParams(std::initializer_list prefix, return builderParams; } +static void errorIfPruned(size_t line, Method *m, const Twine &methodName, + const AttrOrTypeDef &def) { + if (m) + return; + PrintFatalError(def.getLoc(), "Unexpected overlap when generating `" + + methodName + "` for " + def.getName() + + " (from line " + Twine(line) + ")"); +} + +#define ERROR_IF_PRUNED(M, N, O) errorIfPruned(__LINE__, M, N, O) + void DefGen::emitCustomBuilder(const AttrOrTypeBuilder &builder) { // Don't emit a body if there isn't one. auto props = builder.getBody() ? Method::Static : Method::StaticDeclaration; @@ -521,6 +532,10 @@ void DefGen::emitCustomBuilder(const AttrOrTypeBuilder &builder) { returnType = *builderReturnType; Method *m = defCls.addMethod(returnType, "get", props, getCustomBuilderParams({}, builder)); + + // If method is pruned, report error and terminate. + ERROR_IF_PRUNED(m, "get", def); + if (!builder.getBody()) return; @@ -552,6 +567,10 @@ void DefGen::emitCheckedCustomBuilder(const AttrOrTypeBuilder &builder) { getCustomBuilderParams( {{"::llvm::function_ref<::mlir::InFlightDiagnostic()>", "emitError"}}, builder)); + + // If method is pruned, report error and terminate. + ERROR_IF_PRUNED(m, "getChecked", def); + if (!builder.getBody()) return; From cac13aa6e594e59839488a64b97e07ea503611ac Mon Sep 17 00:00:00 2001 From: Justin Kim Date: Thu, 25 Sep 2025 01:04:55 +0900 Subject: [PATCH 2/4] fix : check for pruned builder regardless of body prescence --- mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp b/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp index 4fdde76a613bb..7e8e559baf878 100644 --- a/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp +++ b/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp @@ -3104,8 +3104,8 @@ void OpEmitter::genBuilder() { std::optional 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()); From 3525cc4c834c09164b7d59235a2a1728ea99b057 Mon Sep 17 00:00:00 2001 From: Justin Kim Date: Thu, 25 Sep 2025 13:50:21 +0900 Subject: [PATCH 3/4] feat : add test for duplicated builders --- .../attr-duplicated-builder-error.td | 39 +++++++++++++++++ .../attr-duplicated-custom-builders-error.td | 43 +++++++++++++++++++ 2 files changed, 82 insertions(+) create mode 100644 mlir/test/mlir-tblgen/attr-duplicated-builder-error.td create mode 100644 mlir/test/mlir-tblgen/attr-duplicated-custom-builders-error.td diff --git a/mlir/test/mlir-tblgen/attr-duplicated-builder-error.td b/mlir/test/mlir-tblgen/attr-duplicated-builder-error.td new file mode 100644 index 0000000000000..8ac63cad9c6a8 --- /dev/null +++ b/mlir/test/mlir-tblgen/attr-duplicated-builder-error.td @@ -0,0 +1,39 @@ +// 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 traits = []> + : AttrDef { + 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 { + 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:15:5: error: Unexpected overlap when generating `get` for TestAttr (from line 537) diff --git a/mlir/test/mlir-tblgen/attr-duplicated-custom-builders-error.td b/mlir/test/mlir-tblgen/attr-duplicated-custom-builders-error.td new file mode 100644 index 0000000000000..12615d0c66e97 --- /dev/null +++ b/mlir/test/mlir-tblgen/attr-duplicated-custom-builders-error.td @@ -0,0 +1,43 @@ +// 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 traits = []> + : AttrDef { + 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 { + 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:15:5: error: Unexpected overlap when generating `get` for TestAttr (from line {{.*}}) From 618b571e73183b04f8d04e60f9a2350af83dfab3 Mon Sep 17 00:00:00 2001 From: Justin Kim Date: Sun, 28 Sep 2025 17:37:55 +0900 Subject: [PATCH 4/4] refactor : improve error message for conflicting builders --- mlir/include/mlir/TableGen/Class.h | 4 ++ .../attr-duplicated-builder-error.td | 11 +++- .../attr-duplicated-custom-builders-error.td | 11 +++- mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp | 66 ++++++++++++++----- 4 files changed, 73 insertions(+), 19 deletions(-) diff --git a/mlir/include/mlir/TableGen/Class.h b/mlir/include/mlir/TableGen/Class.h index 10349676625d1..e6bedc7cc896d 100644 --- a/mlir/include/mlir/TableGen/Class.h +++ b/mlir/include/mlir/TableGen/Class.h @@ -789,6 +789,10 @@ class Class { std::forward(args)...); } + const std::vector> &getMethods() const { + return methods; + } + /// Add a new field to the class. Class fields added this way are always /// private. template diff --git a/mlir/test/mlir-tblgen/attr-duplicated-builder-error.td b/mlir/test/mlir-tblgen/attr-duplicated-builder-error.td index 8ac63cad9c6a8..5f1c61a3a505d 100644 --- a/mlir/test/mlir-tblgen/attr-duplicated-builder-error.td +++ b/mlir/test/mlir-tblgen/attr-duplicated-builder-error.td @@ -36,4 +36,13 @@ def Test_TestAttrOp : Op { let assemblyFormat = "$testAttr attr-dict"; } -// CHECK: attr-duplicated-builder-error.td:15:5: error: Unexpected overlap when generating `get` for TestAttr (from line 537) +// 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. diff --git a/mlir/test/mlir-tblgen/attr-duplicated-custom-builders-error.td b/mlir/test/mlir-tblgen/attr-duplicated-custom-builders-error.td index 12615d0c66e97..0e09f667c1ccd 100644 --- a/mlir/test/mlir-tblgen/attr-duplicated-custom-builders-error.td +++ b/mlir/test/mlir-tblgen/attr-duplicated-custom-builders-error.td @@ -40,4 +40,13 @@ def Test_TestAttrOp : Op { let assemblyFormat = "$testAttr attr-dict"; } -// CHECK: attr-duplicated-custom-builders-error.td:15:5: error: Unexpected overlap when generating `get` for TestAttr (from line {{.*}}) +// 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. diff --git a/mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp b/mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp index b27e4cdcd5b38..b9115657d6bf3 100644 --- a/mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp +++ b/mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp @@ -513,16 +513,39 @@ getCustomBuilderParams(std::initializer_list prefix, return builderParams; } -static void errorIfPruned(size_t line, Method *m, const Twine &methodName, - const AttrOrTypeDef &def) { - if (m) - return; - PrintFatalError(def.getLoc(), "Unexpected overlap when generating `" + - methodName + "` for " + def.getName() + - " (from line " + Twine(line) + ")"); +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; } -#define ERROR_IF_PRUNED(M, N, O) errorIfPruned(__LINE__, M, N, O) +static void emitDuplicatedBuilderError(const Method ¤tMethod, + 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. @@ -530,11 +553,16 @@ void DefGen::emitCustomBuilder(const AttrOrTypeBuilder &builder) { StringRef returnType = def.getCppClassName(); if (std::optional 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. - ERROR_IF_PRUNED(m, "get", def); + if (!m) { + auto curMethod = Method(returnType, methodName, props, parameters); + emitDuplicatedBuilderError(curMethod, methodName, defCls, def); + } if (!builder.getBody()) return; @@ -562,14 +590,18 @@ void DefGen::emitCheckedCustomBuilder(const AttrOrTypeBuilder &builder) { StringRef returnType = def.getCppClassName(); if (std::optional 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. - ERROR_IF_PRUNED(m, "getChecked", def); + if (!m) { + auto curMethod = Method(returnType, methodName, props, parameters); + emitDuplicatedBuilderError(curMethod, methodName, defCls, def); + } if (!builder.getBody()) return;