[Mlir-commits] [mlir] [mlir-tblgen] Relax builder ambiguity check (PR #118310)
Thomas Preud'homme
llvmlistbot at llvm.org
Wed Dec 4 02:05:47 PST 2024
https://github.com/RoboTux updated https://github.com/llvm/llvm-project/pull/118310
>From 148d84a29b972f58ff4a00309d10a7055d55f0ec Mon Sep 17 00:00:00 2001
From: Thomas Preud'homme <thomas.preudhomme at arm.com>
Date: Mon, 2 Dec 2024 15:01:21 +0000
Subject: [PATCH 1/3] [mlir-tblgen] Relax builder ambiguity check
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.
---
.../include/mlir/Dialect/Arith/IR/ArithOps.td | 9 -------
mlir/test/mlir-tblgen/op-default-builder.td | 4 ++--
mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp | 24 +++++++++++++++++--
3 files changed, 24 insertions(+), 13 deletions(-)
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;
}
>From 438ecc9b1bd168251538472a1eadb782e949e7cc Mon Sep 17 00:00:00 2001
From: Thomas Preud'homme <thomas.preudhomme at arm.com>
Date: Tue, 3 Dec 2024 14:15:02 +0000
Subject: [PATCH 2/3] Use range for loop
---
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 1507f73e508d88..59ae7777da5675 100644
--- a/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
+++ b/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
@@ -3092,8 +3092,8 @@ void OpEmitter::buildParamList(SmallVectorImpl<MethodParameter> ¶mList,
// 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));
+ for (const auto &arg : op.getArgs()) {
+ auto *namedAttr = dyn_cast<NamedAttribute *>(arg);
if (!namedAttr)
continue;
Attribute attr = namedAttr->attr;
>From 4408ac4ef93ab6e959c5e990e6f348187c740115 Mon Sep 17 00:00:00 2001
From: Thomas Preud'homme <thomas.preudhomme at arm.com>
Date: Wed, 4 Dec 2024 10:01:55 +0000
Subject: [PATCH 3/3] Remove no longer needed Tosa Rescale constructor
A constructor with default value for both input_unsigned and
output_unsigned is now being generated with both wrapped and unwrapped
parameters.
---
mlir/include/mlir/Dialect/Tosa/IR/TosaOps.td | 18 ------------------
1 file changed, 18 deletions(-)
diff --git a/mlir/include/mlir/Dialect/Tosa/IR/TosaOps.td b/mlir/include/mlir/Dialect/Tosa/IR/TosaOps.td
index b1d3a32598c880..f7bd2cb99890a0 100644
--- a/mlir/include/mlir/Dialect/Tosa/IR/TosaOps.td
+++ b/mlir/include/mlir/Dialect/Tosa/IR/TosaOps.td
@@ -1905,24 +1905,6 @@ def Tosa_RescaleOp: Tosa_Op<"rescale", [Pure,
Tosa_Tensor:$output
);
- // Custom builder that does not require optional input_unsigned and
- // output_unsigned.
- let builders = [
- OpBuilder<(ins "::mlir::Type":$output,
- "::mlir::Value":$input,
- "::mlir::IntegerAttr":$input_zp,
- "::mlir::IntegerAttr":$output_zp,
- "::mlir::DenseI32ArrayAttr":$multiplier,
- "::mlir::DenseI8ArrayAttr":$shift,
- "::mlir::BoolAttr":$scale32,
- "::mlir::BoolAttr":$double_round,
- "::mlir::BoolAttr":$per_channel), [{
- auto FalseAttr = BoolAttr::get($_builder.getContext(), false);
- build($_builder, $_state, output, input, input_zp, output_zp, multiplier,
- shift, scale32, double_round, per_channel, FalseAttr, FalseAttr);
- }]>
- ];
-
let assemblyFormat = "operands attr-dict `:` functional-type(operands, results)";
}
More information about the Mlir-commits
mailing list