[Mlir-commits] [mlir] cd36bab - Fix bug for Ops with default valued attributes and successors/variadic regions.

[Mehdi Amini]

Author: Tyler Augustine
Date: 2021-09-22T21:22:31Z
New Revision: cd36bab4ca901199c753e99cb66143eca52ffed8

URL: https://github.com/llvm/llvm-project/commit/cd36bab4ca901199c753e99cb66143eca52ffed8
DIFF: https://github.com/llvm/llvm-project/commit/cd36bab4ca901199c753e99cb66143eca52ffed8.diff

LOG: Fix bug for Ops with default valued attributes and successors/variadic regions.

When both a DefaultValuedAttr and a successor or variadic region was specified, this would generate invalid C++ declaration. There would be the parameter with a default value, followed by the successors/regions, which don't have a default, which is invalid.

Reviewed By: mehdi_amini

Differential Revision: https://reviews.llvm.org/D110205

Added: 
    

Modified: 
    mlir/test/mlir-tblgen/op-attribute.td
    mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

Removed: 
    


################################################################################
diff  --git a/mlir/test/mlir-tblgen/op-attribute.td b/mlir/test/mlir-tblgen/op-attribute.td
index 0440dd165f9bf..02d63c04355b7 100644
--- a/mlir/test/mlir-tblgen/op-attribute.td
+++ b/mlir/test/mlir-tblgen/op-attribute.td
@@ -278,6 +278,30 @@ def MixOperandsAndAttrs : NS_Op<"mix_operands_and_attrs", []> {
   let arguments = (ins F32Attr:$attr, F32:$operand, F32Attr:$otherAttr, F32:$otherArg);
 }
 
+def OpWithDefaultAndRegion : NS_Op<"default_with_region", []> {
+  let arguments = (ins
+          DefaultValuedAttr<BoolAttr, "true">:$dv_bool_attr
+  );
+  let regions = (region VariadicRegion<AnyRegion>:$region);
+}
+
+// We should not have a default attribute in this case.
+
+// DECL-LABEL: OpWithDefaultAndRegion declarations
+// DECL: static void build({{.*}}, bool dv_bool_attr, unsigned regionCount)
+
+def OpWithDefaultAndSuccessor : NS_Op<"default_with_succ", []> {
+  let arguments = (ins
+          DefaultValuedAttr<BoolAttr, "true">:$dv_bool_attr
+  );
+  let successors = (successor VariadicSuccessor<AnySuccessor>:$succ);
+}
+
+// We should not have a default attribute in this case.
+
+// DECL-LABEL: OpWithDefaultAndSuccessor declarations
+// DECL: static void build({{.*}}, bool dv_bool_attr, ::mlir::BlockRange succ)
+
 // DEF-LABEL: MixOperandsAndAttrs definitions
 // DEF-DAG: ::mlir::Value MixOperandsAndAttrs::operand()
 // DEF-DAG: ::mlir::Value MixOperandsAndAttrs::otherArg()

diff  --git a/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp b/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
index 9562e9b1c1f92..2c4cd7a27024c 100644
--- a/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
+++ b/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
@@ -1512,7 +1512,10 @@ void OpEmitter::buildParamList(SmallVectorImpl<OpMethodParameter> &paramList,
 
   // Add parameters for all arguments (operands and attributes).
   int defaultValuedAttrStartIndex = op.getNumArgs();
-  if (attrParamKind == AttrParamKind::UnwrappedValue) {
+  // Successors and variadic regions go at the end of the parameter list, so no
+  // default arguments are possible.
+  bool hasTrailingParams = op.getNumSuccessors() || op.getNumVariadicRegions();
+  if (attrParamKind == AttrParamKind::UnwrappedValue && !hasTrailingParams) {
     // Calculate the start index from which we can attach default values in the
     // builder declaration.
     for (int i = op.getNumArgs() - 1; i >= 0; --i) {