[mlir-tblgen] Relax builder ambiguity check #118310
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The mlir-tblgen tool prevents the parameter of the build() constructor for the first default-valued attribute of an operation from having a default value to avoid ambiguity with the corresponding build() constructor taking unwrapped value. However it does so even when earlier wrapped unwrappable attribute would lift the ambiguity. This commit relax the logic accordingly, which allows to remove a manual constructor in Arith dialect.
|
@llvm/pr-subscribers-mlir-tosa @llvm/pr-subscribers-mlir-arith Author: Thomas Preud'homme (RoboTux) ChangesThe mlir-tblgen tool prevents the parameter of the build() constructor Full diff: https://github.com/llvm/llvm-project/pull/118310.diff 3 Files Affected:
diff --git a/mlir/include/mlir/Dialect/Arith/IR/ArithOps.td b/mlir/include/mlir/Dialect/Arith/IR/ArithOps.td
index 19a5e13a5d755d..2f71caaa593a6c 100644
--- a/mlir/include/mlir/Dialect/Arith/IR/ArithOps.td
+++ b/mlir/include/mlir/Dialect/Arith/IR/ArithOps.td
@@ -1550,15 +1550,6 @@ def Arith_CmpFOp : Arith_CompareOp<"cmpf",
static arith::CmpFPredicate getPredicateByName(StringRef name);
}];
- let builders = [
- OpBuilder<(ins "::mlir::arith::CmpFPredicateAttr":$predicate,
- "Value":$lhs, "Value":$rhs), [{
- build($_builder, $_state, predicate, lhs, rhs,
- mlir::arith::FastMathFlagsAttr::get($_builder.getContext(),
- mlir::arith::FastMathFlags::none));
- }]>
- ];
-
let hasFolder = 1;
let hasCanonicalizer = 1;
let assemblyFormat = [{ $predicate `,` $lhs `,` $rhs (`fastmath` `` $fastmath^)?
diff --git a/mlir/test/mlir-tblgen/op-default-builder.td b/mlir/test/mlir-tblgen/op-default-builder.td
index 82386f245bc5dc..9881f925a19220 100644
--- a/mlir/test/mlir-tblgen/op-default-builder.td
+++ b/mlir/test/mlir-tblgen/op-default-builder.td
@@ -31,9 +31,9 @@ def AOp : NS_Op<"a_op", []> {
// CHECK-LABEL: AOp declarations
// Note: `cAttr` below could be conditionally optional and so the generation is
// currently conservative.
-// CHECK-DAG: ::mlir::Value lhs, some-attr-kind aAttr, some-attr-kind bAttr, /*optional*/some-attr-kind cAttr, /*optional*/some-attr-kind dAttr);
+// CHECK-DAG: ::mlir::Value lhs, some-attr-kind aAttr, some-attr-kind bAttr, /*optional*/some-attr-kind cAttr, /*optional*/some-attr-kind dAttr = nullptr);
// CHECK-DAG: ::mlir::Value lhs, some-return-type aAttr, some-return-type bAttr, /*optional*/some-attr-kind cAttr, /*optional*/some-return-type dAttr = 7.2);
-// CHECK-DAG: ::mlir::TypeRange resultTypes, ::mlir::Value lhs, some-attr-kind aAttr, some-attr-kind bAttr, /*optional*/some-attr-kind cAttr, /*optional*/some-attr-kind dAttr);
+// CHECK-DAG: ::mlir::TypeRange resultTypes, ::mlir::Value lhs, some-attr-kind aAttr, some-attr-kind bAttr, /*optional*/some-attr-kind cAttr, /*optional*/some-attr-kind dAttr = nullptr);
// CHECK-DAG: ::mlir::TypeRange resultTypes, ::mlir::Value lhs, some-return-type aAttr, some-return-type bAttr, /*optional*/some-attr-kind cAttr, /*optional*/some-return-type dAttr = 7.2);
def BOp : NS_Op<"b_op", []> {
diff --git a/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp b/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
index 9badb7aa163a60..1507f73e508d88 100644
--- a/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
+++ b/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
@@ -3088,6 +3088,26 @@ void OpEmitter::buildParamList(SmallVectorImpl<MethodParameter> ¶mList,
defaultValuedAttrStartIndex = i;
}
}
+
+ // Check if parameters besides default valued one are enough to distinguish
+ // between builders with wrapped and unwrapped arguments.
+ bool hasBuilderAmbiguity = true;
+ for (int i = 0; i < op.getNumArgs(); ++i) {
+ auto *namedAttr = dyn_cast<NamedAttribute *>(op.getArg(i));
+ if (!namedAttr)
+ continue;
+ Attribute attr = namedAttr->attr;
+ if (attr.hasDefaultValue() || attr.isDerivedAttr())
+ continue;
+
+ if (attrParamKind != AttrParamKind::WrappedAttr ||
+ !canUseUnwrappedRawValue(attr))
+ continue;
+
+ hasBuilderAmbiguity = false;
+ break;
+ }
+
// Avoid generating build methods that are ambiguous due to default values by
// requiring at least one attribute.
if (defaultValuedAttrStartIndex < op.getNumArgs()) {
@@ -3098,9 +3118,9 @@ void OpEmitter::buildParamList(SmallVectorImpl<MethodParameter> ¶mList,
cast<NamedAttribute *>(op.getArg(defaultValuedAttrStartIndex));
Attribute attr = namedAttr->attr;
if ((attrParamKind == AttrParamKind::WrappedAttr &&
- canUseUnwrappedRawValue(attr)) ||
+ canUseUnwrappedRawValue(attr) && hasBuilderAmbiguity) ||
(attrParamKind == AttrParamKind::UnwrappedValue &&
- !canUseUnwrappedRawValue(attr))) {
+ !canUseUnwrappedRawValue(attr) && hasBuilderAmbiguity)) {
++defaultValuedAttrStartIndex;
defaultValuedAttrLikeStartIndex = defaultValuedAttrStartIndex;
}
|
|
@llvm/pr-subscribers-mlir-core Author: Thomas Preud'homme (RoboTux) ChangesThe mlir-tblgen tool prevents the parameter of the build() constructor Full diff: https://github.com/llvm/llvm-project/pull/118310.diff 3 Files Affected:
diff --git a/mlir/include/mlir/Dialect/Arith/IR/ArithOps.td b/mlir/include/mlir/Dialect/Arith/IR/ArithOps.td
index 19a5e13a5d755d..2f71caaa593a6c 100644
--- a/mlir/include/mlir/Dialect/Arith/IR/ArithOps.td
+++ b/mlir/include/mlir/Dialect/Arith/IR/ArithOps.td
@@ -1550,15 +1550,6 @@ def Arith_CmpFOp : Arith_CompareOp<"cmpf",
static arith::CmpFPredicate getPredicateByName(StringRef name);
}];
- let builders = [
- OpBuilder<(ins "::mlir::arith::CmpFPredicateAttr":$predicate,
- "Value":$lhs, "Value":$rhs), [{
- build($_builder, $_state, predicate, lhs, rhs,
- mlir::arith::FastMathFlagsAttr::get($_builder.getContext(),
- mlir::arith::FastMathFlags::none));
- }]>
- ];
-
let hasFolder = 1;
let hasCanonicalizer = 1;
let assemblyFormat = [{ $predicate `,` $lhs `,` $rhs (`fastmath` `` $fastmath^)?
diff --git a/mlir/test/mlir-tblgen/op-default-builder.td b/mlir/test/mlir-tblgen/op-default-builder.td
index 82386f245bc5dc..9881f925a19220 100644
--- a/mlir/test/mlir-tblgen/op-default-builder.td
+++ b/mlir/test/mlir-tblgen/op-default-builder.td
@@ -31,9 +31,9 @@ def AOp : NS_Op<"a_op", []> {
// CHECK-LABEL: AOp declarations
// Note: `cAttr` below could be conditionally optional and so the generation is
// currently conservative.
-// CHECK-DAG: ::mlir::Value lhs, some-attr-kind aAttr, some-attr-kind bAttr, /*optional*/some-attr-kind cAttr, /*optional*/some-attr-kind dAttr);
+// CHECK-DAG: ::mlir::Value lhs, some-attr-kind aAttr, some-attr-kind bAttr, /*optional*/some-attr-kind cAttr, /*optional*/some-attr-kind dAttr = nullptr);
// CHECK-DAG: ::mlir::Value lhs, some-return-type aAttr, some-return-type bAttr, /*optional*/some-attr-kind cAttr, /*optional*/some-return-type dAttr = 7.2);
-// CHECK-DAG: ::mlir::TypeRange resultTypes, ::mlir::Value lhs, some-attr-kind aAttr, some-attr-kind bAttr, /*optional*/some-attr-kind cAttr, /*optional*/some-attr-kind dAttr);
+// CHECK-DAG: ::mlir::TypeRange resultTypes, ::mlir::Value lhs, some-attr-kind aAttr, some-attr-kind bAttr, /*optional*/some-attr-kind cAttr, /*optional*/some-attr-kind dAttr = nullptr);
// CHECK-DAG: ::mlir::TypeRange resultTypes, ::mlir::Value lhs, some-return-type aAttr, some-return-type bAttr, /*optional*/some-attr-kind cAttr, /*optional*/some-return-type dAttr = 7.2);
def BOp : NS_Op<"b_op", []> {
diff --git a/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp b/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
index 9badb7aa163a60..1507f73e508d88 100644
--- a/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
+++ b/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
@@ -3088,6 +3088,26 @@ void OpEmitter::buildParamList(SmallVectorImpl<MethodParameter> ¶mList,
defaultValuedAttrStartIndex = i;
}
}
+
+ // Check if parameters besides default valued one are enough to distinguish
+ // between builders with wrapped and unwrapped arguments.
+ bool hasBuilderAmbiguity = true;
+ for (int i = 0; i < op.getNumArgs(); ++i) {
+ auto *namedAttr = dyn_cast<NamedAttribute *>(op.getArg(i));
+ if (!namedAttr)
+ continue;
+ Attribute attr = namedAttr->attr;
+ if (attr.hasDefaultValue() || attr.isDerivedAttr())
+ continue;
+
+ if (attrParamKind != AttrParamKind::WrappedAttr ||
+ !canUseUnwrappedRawValue(attr))
+ continue;
+
+ hasBuilderAmbiguity = false;
+ break;
+ }
+
// Avoid generating build methods that are ambiguous due to default values by
// requiring at least one attribute.
if (defaultValuedAttrStartIndex < op.getNumArgs()) {
@@ -3098,9 +3118,9 @@ void OpEmitter::buildParamList(SmallVectorImpl<MethodParameter> ¶mList,
cast<NamedAttribute *>(op.getArg(defaultValuedAttrStartIndex));
Attribute attr = namedAttr->attr;
if ((attrParamKind == AttrParamKind::WrappedAttr &&
- canUseUnwrappedRawValue(attr)) ||
+ canUseUnwrappedRawValue(attr) && hasBuilderAmbiguity) ||
(attrParamKind == AttrParamKind::UnwrappedValue &&
- !canUseUnwrappedRawValue(attr))) {
+ !canUseUnwrappedRawValue(attr) && hasBuilderAmbiguity)) {
++defaultValuedAttrStartIndex;
defaultValuedAttrLikeStartIndex = defaultValuedAttrStartIndex;
}
|
kuhar
reviewed
Dec 2, 2024
|
Seems reasonable conceptually aside the nits above |
…ap_tablegen_check
A constructor with default value for both input_unsigned and output_unsigned is now being generated with both wrapped and unwrapped parameters.
jpienaar
approved these changes
Dec 5, 2024
|
I see some failures MHLO side ( out-of-line definition of 'build' does not match any declaration ), so seems like this is not just avoiding ambiguity but may also skipping generating some build methods that was generated before. |
|
Yes, so this results in generating different number of build methods than before in the header, but not the bodies. You can check in build of tensorflow/compiler/xla/mlir_hlo/_objs/mlir_hlo/hlo_ops |
jpienaar
requested changes
Dec 6, 2024
jpienaar
approved these changes
Dec 6, 2024
There was a problem hiding this comment.
OK, I had time to look more. The confusing thing was the user provided builder was not preferred but that's not new here. Post that only 1 change I noticed, seems a different build method being called which resulted in DefaultValuedOptionalAttr not being set. But that doesn't seem to be a difference introduce here.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The mlir-tblgen tool prevents the parameter of the build() constructor
for the first default-valued attribute of an operation from having a
default value to avoid ambiguity with the corresponding build()
constructor taking unwrapped value. However it does so even when earlier
wrapped unwrappable attribute would lift the ambiguity. This commit
relax the logic accordingly, which allows to remove a manual constructor
in Arith dialect.