[Mlir-commits] [mlir] [mlir-tblgen] Relax builder ambiguity check (PR #118310)

[llvmlistbot at llvm.org]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-mlir-core

Author: Thomas Preud'homme (RoboTux)

<details>
<summary>Changes</summary>

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.


---
Full diff: https://github.com/llvm/llvm-project/pull/118310.diff


3 Files Affected:

- (modified) mlir/include/mlir/Dialect/Arith/IR/ArithOps.td (-9) 
- (modified) mlir/test/mlir-tblgen/op-default-builder.td (+2-2) 
- (modified) mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp (+22-2) 


``````````diff
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> &paramList,
       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> &paramList,
         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;
     }

``````````

</details>


https://github.com/llvm/llvm-project/pull/118310