[mlir][mlir-tblgen] Emit correct error message if method is pruned #160334
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-mlir-core @llvm/pr-subscribers-mlir Author: Justin Kim (JustinKim98) ChangesThis PR fixes issue #160227 Changes
Full diff: https://github.com/llvm/llvm-project/pull/160334.diff 2 Files Affected:
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<MethodParameter> 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;
|
|
@llvm/pr-subscribers-mlir-tosa Author: Justin Kim (JustinKim98) ChangesThis PR fixes issue #160227 Changes
Full diff: https://github.com/llvm/llvm-project/pull/160334.diff 2 Files Affected:
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<MethodParameter> 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;
|
|
Asking for review from |
|
From I wonder if there's some specific reason that void OpEmitter::genBuilder() {
// Handle custom builders if provided.
for (const Builder &builder : op.getBuilders()) {
SmallVector<MethodParameter> arguments = getBuilderSignature(builder);
std::optional<StringRef> body = builder.getBody();
auto properties = body ? Method::Static : Method::StaticDeclaration;
auto *method = opClass.addMethod("void", "build", properties, arguments);
// Why is this checked only if body is present?
if (body)
ERROR_IF_PRUNED(method, "build", op);
if (method)
method->setDeprecated(builder.getDeprecatedMessage()); |
|
I don't know, but is anything failing in MLIR if you fix this filtering? |
I couldn't observe anything fail after removing the filtering. (removing |
|
This pr is ready for review |
| let assemblyFormat = "$testAttr attr-dict"; | ||
| } | ||
|
|
||
| // CHECK: attr-duplicated-builder-error.td:15:5: error: Unexpected overlap when generating `get` for TestAttr (from line 537) |
There was a problem hiding this comment.
What is the "from line 537" referring to here?
There was a problem hiding this comment.
It's line number of error location in AttrOrTypeDefGen.cpp.
I followed this to be consistent with error handling logic in OpDefinitionsGen.cpp.
However, if you feel error message looks terse, I can improve it to provide more information, and omit line number of the original source code.
There was a problem hiding this comment.
Yes that seems preferable for the end-user. Thanks!
| let assemblyFormat = "$testAttr attr-dict"; | ||
| } | ||
|
|
||
| // CHECK: attr-duplicated-custom-builders-error.td:15:5: error: Unexpected overlap when generating `get` for TestAttr (from line {{.*}}) |
There was a problem hiding this comment.
The error is quite terse, can we improve it? Can we point at the two methods that are shadowing each other?
There was a problem hiding this comment.
I'll try to do so, but custom method itself doesn't store the exact location in the file where it was defined.
But I can still point to location of let builders = ...
|
Update :
|
|
@JustinKim98 Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
…lvm#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 llvm#160227 --------- Co-authored-by: Justin Kim <jaewoo.kim@hyperaccel.ai>
This PR fixes issue #160227
Changes
emitCustomBuilderandemitCheckedCustomBuildermlir-tblgenwith--gen-attrdef-declswould segmentation fault if custom builder is provided with its body, but if method is pruned out due to duplication with other builders.OpDefinitionsGen.cppTosaTypesBase.tdUpdate
genBuilderfromOpDefinitions.cppto check for pruned custom-builder regardless of presence of its body.mlir-tblgenreproducing the error when attribute builder is duplicated.