[mlir] Expand default arg builders to wrapped attributes.

[jpienaar]

Summary

Previously only unwrapped values were considered for default values in
builders, expand to Attributes given the change to populate defaults. To avoid
overlap between wrapped and unwrapped, the starting index is incremented
depending on if the smallest starting index for default valued args can be
materialized in unwrapped form or not.

Given the above limitation this is pretty small change.

optional is unfortunately meant to also represent conditionally set which
means we cannot allow nullptr for all Attributes marked optional at the moment.

[Mogball]

nice

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

2900

can you inline this variable?

2918

does this work?

2923

please drop trivial braces

2982

please drop trivial braces (I know they were already here)

[jpienaar]

Address comments and rebase

Couple of more custom builders got added that overlap that needs to be addressed first.

mlir/test/mlir-tblgen/op-attribute.td

 
 // DEF-LABEL: DefaultDictAttrOp definitions
 // DEF: if (!attributes.get(attrNames[0]))
 // DEF:   attributes.append(attrNames[0], odsBuilder.getDictionaryAttr({}));
 // DEF: if (!attributes.get(attrNames[1]))
 // DEF:   attributes.append(attrNames[1], odsBuilder.getDictionaryAttr(getDefaultDictAttrs(odsBuilder)));
 
 // DECL-LABEL: DefaultDictAttrOp declarations
-// DECL: build(::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &odsState, ::mlir::DictionaryAttr empty, ::mlir::DictionaryAttr non_empty)
+// DECL: build(::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &odsState, ::mlir::DictionaryAttr empty = nullptr, ::mlir::DictionaryAttr non_empty = nullptr)
 
 
 // Test derived type attr.
 // ---
 def DerivedTypeAttrOp : NS_Op<"derived_type_attr_op", []> {
   let results = (outs AnyTensor:$output);
   DerivedTypeAttr element_dtype = DerivedTypeAttr<"return output().getType();">;
 }

mlir/test/mlir-tblgen/op-default-builder.td

+// RUN: mlir-tblgen -gen-op-decls -I %S/../../include %s | FileCheck %s
+
+include "mlir/IR/AttrTypeBase.td"
+include "mlir/IR/EnumAttr.td"
+include "mlir/IR/OpBase.td"
+
+def Test_Dialect : Dialect {
+  let name = "test";
+  let cppNamespace = "foobar";
+}
+class NS_Op<string mnemonic, list<Trait> traits> :
+    Op<Test_Dialect, mnemonic, traits>;
+
+def SomeAttr : Attr<CPred<"some-condition">, "some attribute kind"> {
+  let storageType = "some-attr-kind";
+  let returnType = "some-return-type";
+  let convertFromStorage = "$_self.some-convert-from-storage()";
+  let constBuilderCall = "some-const-builder-call($_builder, $0)";
+}
+
+def AOp : NS_Op<"a_op", []> {
+  let arguments = (ins
+      FloatLike:$lhs,
+      SomeAttr:$aAttr,
+      DefaultValuedAttr<SomeAttr, "4.2">:$bAttr,
+      OptionalAttr<SomeAttr>:$cAttr,
+      DefaultValuedOptionalAttr<SomeAttr, "7.2">:$dAttr
+  );
+}
+
+// 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-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-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", []> {
+  let arguments = (ins
+      DefaultValuedAttr<SomeAttr, "6.2">:$aAttr,
+      DefaultValuedAttr<SomeAttr, "4.2">:$bAttr
+  );
+}
+
+// Verify that non-overlapping builders created where all could be elided.
+// CHECK-LABEL: BOp declarations
+// CHECK-DAG: some-attr-kind aAttr, some-attr-kind bAttr = nullptr);
+// CHECK-DAG: some-return-type aAttr = 6.2, some-return-type bAttr = 4.2);
+// CHECK-DAG: ::mlir::TypeRange resultTypes, some-attr-kind aAttr, some-attr-kind bAttr = nullptr);
+// CHECK-DAG: ::mlir::TypeRange resultTypes, some-return-type aAttr = 6.2, some-return-type bAttr = 4.2);
+
+def COp : NS_Op<"c_op", []> {
+  let arguments = (ins
+    FloatLike:$value,
+    OptionalAttr<SymbolRefArrayAttr>:$ag,
+    OptionalAttr<SymbolRefArrayAttr>:$as,
+    OptionalAttr<SymbolRefArrayAttr>:$nos,
+    OptionalAttr<I64Attr>:$al,
+    UnitAttr:$vo,
+    UnitAttr:$non
+  );
+}
+
+// CHECK-LABEL: COp declarations
+// Note: `al` below could be conditionally optional and so the generation is
+// currently conservative.
+// CHECK-DAG: ::mlir::Value value, /*optional*/::mlir::ArrayAttr ag, /*optional*/::mlir::ArrayAttr as, /*optional*/::mlir::ArrayAttr nos, /*optional*/::mlir::IntegerAttr al, /*optional*/::mlir::UnitAttr vo, /*optional*/::mlir::UnitAttr non = nullptr);
+// CHECK-DAG: ::mlir::Value value, /*optional*/::mlir::ArrayAttr ag, /*optional*/::mlir::ArrayAttr as, /*optional*/::mlir::ArrayAttr nos, /*optional*/::mlir::IntegerAttr al, /*optional*/bool vo = false, /*optional*/bool non = false);
+// CHECK-DAG: ::mlir::TypeRange resultTypes, ::mlir::Value value, /*optional*/::mlir::ArrayAttr ag, /*optional*/::mlir::ArrayAttr as, /*optional*/::mlir::ArrayAttr nos, /*optional*/::mlir::IntegerAttr al, /*optional*/::mlir::UnitAttr vo, /*optional*/::mlir::UnitAttr non = nullptr);
+// CHECK-DAG: ::mlir::TypeRange resultTypes, ::mlir::Value value, /*optional*/::mlir::ArrayAttr ag, /*optional*/::mlir::ArrayAttr as, /*optional*/::mlir::ArrayAttr nos, /*optional*/::mlir::IntegerAttr al, /*optional*/bool vo = false, /*optional*/bool non = false);

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

void OpEmitter::buildParamList(SmallVectorImpl<MethodParameter> &paramList,
   } break;
   }
 
   // Add parameters for all arguments (operands and attributes).
   int defaultValuedAttrStartIndex = op.getNumArgs();
   // 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) {
+  if (!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) {
       auto *namedAttr =
           llvm::dyn_cast_if_present<tblgen::NamedAttribute *>(op.getArg(i));
-      if (!namedAttr || !namedAttr->attr.hasDefaultValue())
+      if (!namedAttr)
         break;
 
-      if (!canUseUnwrappedRawValue(namedAttr->attr))
+      Attribute attr = namedAttr->attr;
+      // TODO: Currently we can't differentiate between optional meaning do not
+      // verify/not always error if missing or optional meaning need not be
+      // specified in builder. Expand isOptional once we can differentiate.
+      if (!attr.hasDefaultValue() && !attr.isDerivedAttr())
         break;
 
       // Creating an APInt requires us to provide bitwidth, value, and
       // signedness, which is complicated compared to others. Similarly
       // for APFloat.
       // TODO: Adjust the 'returnType' field of such attributes
       // to support them.
       StringRef retType = namedAttr->attr.getReturnType();
       if (retType == "::llvm::APInt" || retType == "::llvm::APFloat")
         break;
 
       defaultValuedAttrStartIndex = i;
     }
   }
+  // Avoid generating build methods that are ambiguous due to default values by
+  // requiring at least one attribute.
+  if (defaultValuedAttrStartIndex < op.getNumArgs()) {
+    // TODO: This should have been possible as a cast<NamedAttribute> but
+    // required template instantiations is not yet defined for the tblgen helper
+    // classes.
+    auto *namedAttr =
+        cast<NamedAttribute *>(op.getArg(defaultValuedAttrStartIndex));
+    Attribute attr = namedAttr->attr;
+    if ((attrParamKind == AttrParamKind::WrappedAttr &&
+         canUseUnwrappedRawValue(attr)) ||
+        (attrParamKind == AttrParamKind::UnwrappedValue &&
+         !canUseUnwrappedRawValue(attr)))
+      ++defaultValuedAttrStartIndex;
+  }
 
   /// Collect any inferred attributes.
   for (const NamedTypeConstraint &operand : op.getOperands()) {
     if (operand.isVariadicOfVariadic()) {
       inferredAttributes.insert(
           operand.constraint.getVariadicOfVariadicSegmentSizeAttr());
     }
   }
case AttrParamKind::UnwrappedValue:
         type = attr.getReturnType();
       else
         type = attr.getStorageType();
       break;
     }
 
     // Attach default value if requested and possible.
     std::string defaultValue;
+    if (i >= defaultValuedAttrStartIndex) {
       if (attrParamKind == AttrParamKind::UnwrappedValue &&
-        i >= defaultValuedAttrStartIndex) {
+          canUseUnwrappedRawValue(attr))
         defaultValue += attr.getDefaultValue();
+      else
+        defaultValue += "nullptr";
     }
     paramList.emplace_back(type, namedAttr.name, StringRef(defaultValue),
                            attr.isOptional());
   }
 
   /// Insert parameters for each successor.
   for (const NamedSuccessor &succ : op.getSuccessors()) {
     StringRef type =