[mlir][ods] Add option to elide attribute printing when the value is the default

[jfurtek]

Summary

This diff adds a bit to the tblgen Attr class that causes the tblgenerated
print() function to skip printing a DefaultValuedAttr attribute when the value
is equal to the default.

This feature will reduce the amount of custom printing code that needs to be
written by users a relatively common scenario. As a motivating example, for the
fastmath flags in the LLVMIR dialect, we would prefer to print this:

%0 = llvm.fadd %arg0, %arg1 : f32

instead of this:

%0 = llvm.fadd %arg0, %arg1 {fastmathFlags = #llvm.fastmath<none>} : f32

The (in-progress) fastmath support in the arith dialect will use this
feature, as will the LLVMIR dialect. It is expected that other dialects will
also make use of the fastmath attribute, and these dialects can opt-in for the
same behavior. Adding support to tblgen will allow other attributes to
leverage this capability as well.

[rriddle]

Haven't looked deeply but this kind of feels weird, why wouldn't we just always elide the default?

[jfurtek]

Adding @Mogball since this is based heavily on his diff for optional parameters (https://reviews.llvm.org/D133816)

[jfurtek]

Haven't looked deeply but this kind of feels weird, why wouldn't we just always elide the default?

I'm happy to make that the case - it felt presumptuous for me to assume no one would want to see the value in the IR.

Maybe there are situations where the default is non-obvious, and retaining the attribute in the IR prevents people from having to look up the default.

[rkayaith]

btw D134993 does something similar, though it's for the custom assembly format syntax. so for the fastmath example you could do something like:

let assemblyFormat = "$lhs `,` $rhs (`fastmath` `(` $fastmathFlags^ `)`)? attr-dict `:` type($res)";

which would print/parse as:

llvm.fadd %arg0, %arg1 fastmath(<fast>) : f32
llvm.fadd %arg0, %arg1 : f32  // same as fastmath(<>)

[jfurtek]

@rriddle @mehdi_amini Is there any chance we can come to a resolution on this? (I'd like to land the fastmath support - which has a default-valued attribute - and a couple of others have expressed interest in it.)

It seems to me that the options are:

1. Abandon this diff and do nothing: Operations that want to elide printing default attributes can write custom handlers on a per-operation basis.
2. Use this diff: a tblgen bit is added on a per-attribute basis. Eliding is "opt-in" so as not to change any existing behavior.
3. Update this diff to "elide-by-default" instead of "opt-in": as suggested by River.
4. Leverage optional group anchor behavior as in https://reviews.llvm.org/D134993 when reviewed/landed

[rriddle]

In D135398#3847619, @jfurtek wrote:

@rriddle @mehdi_amini Is there any chance we can come to a resolution on this? (I'd like to land the fastmath support - which has a default-valued attribute - and a couple of others have expressed interest in it.)

It seems to me that the options are:

1. Abandon this diff and do nothing: Operations that want to elide printing default attributes can write custom handlers on a per-operation basis.
2. Use this diff: a tblgen bit is added on a per-attribute basis. Eliding is "opt-in" so as not to change any existing behavior.
3. Update this diff to "elide-by-default" instead of "opt-in": as suggested by River.
4. Leverage optional group anchor behavior as in https://reviews.llvm.org/D134993 when reviewed/landed

I don't think this bit should be on attributes at all (so -1 to 2), it's specific to operation formats. My comment was directed at the fact that I think we could/should elide default valued attributes from operation dictionaries by default. Otherwise, 4 seems like an easy way to make progress. IIRC we already switched over to populating default attributes on operation construction anyways.

[jpienaar]

+1 to operation format rather than attribute specific.

[jfurtek]

I don't think this bit should be on attributes at all (so -1 to 2),

...

+1 to operation format rather than attribute specific.

Why is this preferable?

If I think in terms of a fastmath attribute, there may eventually be 30 or 40 operations that use the attribute. Some of them may not have a custom assembly format, but if I understand correctly the suggestion here, they will have to now if they don't want to print fastmath<none>.

Conceptually, to me, the idea that fastmath<none> is not "useful" information is a property of the attribute itself. (Operations can override via tablegen attribute class inheritance.)

I think we could/should elide default valued attributes from operation dictionaries by default.

I can change this diff to avoid checking the elidePrintingDefaultValue bit on attributes, but it seems like this would break some tests. Would that be acceptable?

I would like to avoid having to pepper custom assembly on every op that wants to use the fastmath attribute.

[jfurtek]

Abandoning for now - creating a separate diff that removes the tblgen option bit for default-valued attributes.

mlir/include/mlir/IR/OpBase.td

class Attr<Pred condition, string summary = ""> :
   // TOOD(b/132458159): deduplicate the fields in attribute wrapper classes.
   Attr baseAttr = ?;
 
   // The fully-qualified C++ namespace where the generated class lives.
   string cppNamespace = "";
 
   // The full description of this attribute.
   string description = "";
+
+  // If set, the attribute will not be printed if it is equal to the default
+  // value.
+  bit elidePrintingDefaultValue = 0;
 }
 
 // An attribute of a specific dialect.
 class DialectAttr<Dialect d, Pred condition, string summary = ""> :
     Attr<condition, summary> {
   Dialect dialect = d;
   let cppNamespace = d.cppNamespace;
 }
 
 //===----------------------------------------------------------------------===//
 // Attribute modifier definition
 
 // Decorates an attribute to have an (unvalidated) default value if not present.
-class DefaultValuedAttr<Attr attr, string val> :
+class DefaultValuedAttr<Attr attr, string val,
+                        bit elideDefaultPrint = 0> :
     Attr<attr.predicate, attr.summary> {
   // Construct this attribute with the input attribute and change only
   // the default value.
   // Note: this has to be kept up to date with Attr above.
   let storageType = attr.storageType;
   let returnType = attr.returnType;
   let convertFromStorage = attr.convertFromStorage;
   let constBuilderCall = attr.constBuilderCall;
   let defaultValue = val;
   let valueType = attr.valueType;
 
   let baseAttr = attr;
+  let elidePrintingDefaultValue = elideDefaultPrint;
 }
 
 // Decorates an optional attribute to have an (unvalidated) default value
 // return by ODS generated accessors if not present.
-class DefaultValuedOptionalAttr<Attr attr, string val> :
+class DefaultValuedOptionalAttr<Attr attr, string val,
+                                bit elideDefaultPrint = 0> :
     Attr<attr.predicate, attr.summary> {
   // Construct this attribute with the input attribute and change only
   // the default value.
   // Note: this has to be kept up to date with Attr above.
   let storageType = attr.storageType;
   let returnType = attr.returnType;
   let convertFromStorage = attr.convertFromStorage;
   let constBuilderCall = attr.constBuilderCall;
   let defaultValue = val;
   let valueType = attr.valueType;
   let isOptional = 1;
 
   let baseAttr = attr;
+  let elidePrintingDefaultValue = elideDefaultPrint;
 }
 
 // Decorates an attribute as optional. The return type of the generated
 // attribute accessor method will be Optional<>.
 class OptionalAttr<Attr attr> : Attr<attr.predicate, attr.summary> {
   // Rewrite the attribute to be optional.
   // Note: this has to be kept up to date with Attr above.
   let storageType = attr.storageType;

mlir/include/mlir/TableGen/Attribute.h

public:
   // derived attribute.
   StringRef getDerivedCodeBody() const;
 
   // Returns the dialect for the attribute if defined.
   Dialect getDialect() const;
 
   // Returns the TableGen definition this Attribute was constructed from.
   const llvm::Record &getDef() const;
+
+  // Returns true if this attribute should not be printed when the attribute
+  // value is equal to the default value.
+  bool elidePrintingDefaultValue() const;
 };
 
 // Wrapper class providing helper methods for accessing MLIR constant attribute
 // defined in TableGen. This class should closely reflect what is defined as
 // class `ConstantAttr` in TableGen.
 class ConstantAttr {
 public:
   explicit ConstantAttr(const llvm::DefInit *init);

mlir/lib/TableGen/Attribute.cpp

if (record && record->getValue()) {
     if (DefInit *init = dyn_cast<DefInit>(record->getValue()))
       return Dialect(init->getDef());
   }
   return Dialect(nullptr);
 }
 
 const llvm::Record &Attribute::getDef() const { return *def; }
 
+bool Attribute::elidePrintingDefaultValue() const {
+  return def->getValueAsBit("elidePrintingDefaultValue");
+}
+
 ConstantAttr::ConstantAttr(const DefInit *init) : def(init->getDef()) {
   assert(def->isSubClassOf("ConstantAttr") &&
          "must be subclass of TableGen 'ConstantAttr' class");
 }
 
 Attribute ConstantAttr::getAttribute() const {
   return Attribute(def->getValueAsDef("attr"));
 }

mlir/test/IR/attribute.mlir

 func.func @wrong_shape_fail() {
   // expected-error @+1 {{failed to satisfy constraint: 32-bit signless int elements attribute of shape [2]}}
   "test.ranked_int_elements_attr"() {
     matrix_i64_attr = dense<6> : tensor<4x8xi64>,
     vector_i32_attr = dense<5> : tensor<i32>
   } : () -> ()
   return
 }
+
+// -----
+
+//===----------------------------------------------------------------------===//
+// Test DefaultValuedAttr Printing
+//===----------------------------------------------------------------------===//
+
+// CHECK-LABEL: @default_value_printing
+func.func @default_value_printing(%arg0 : i32) {
+  // The attribute SHOULD be printed regardless of equality with the default (0)
+  // CHECK: test.default_value_print {value_with_default = 0 : i32} %arg0
+  "test.default_value_print"(%arg0) {"value_with_default" = 0 : i32} : (i32) -> ()
+  // The attribute SHOULD NOT be printed because elidePrintingDefaultValue=1
+  // CHECK: test.default_value_no_print %arg0
+  "test.default_value_no_print"(%arg0) {"value_with_default" = 0 : i32} : (i32) -> ()
+  // The attribute SHOULD be printed because is is not equal to the default
+  // CHECK: test.default_value_no_print {value_with_default = 1 : i32} %arg0
+  "test.default_value_no_print"(%arg0) {"value_with_default" = 1 : i32} : (i32) -> ()
+  return
+}

mlir/test/lib/Dialect/Test/TestOps.td

def TestDefaultStrAttrHasValueOp : TEST_Op<"has_str_value"> {
   let arguments = (ins DefaultValuedStrAttr<StrAttr, "">:$value);
   let assemblyFormat = "attr-dict";
 }
 
 def : Pat<(TestDefaultStrAttrNoValueOp $value),
           (TestDefaultStrAttrHasValueOp ConstantStrAttr<StrAttr, "foo">)>;
 
 //===----------------------------------------------------------------------===//
+// Test Ops with Default-Valued Attributes and Differing Print Settings
+//===----------------------------------------------------------------------===//
+
+def TestDefaultAttrPrintOp : TEST_Op<"default_value_print"> {
+  let arguments = (ins DefaultValuedAttr<I32Attr, "0">:$value_with_default,
+                   I32:$operand);
+  let assemblyFormat = "attr-dict $operand";
+}
+
+def TestDefaultAttrNoPrintOp : TEST_Op<"default_value_no_print"> {
+  let arguments = (ins DefaultValuedAttr<I32Attr, "0", 1>:$value_with_default,
+                   I32:$operand);
+  let assemblyFormat = "attr-dict $operand";
+}
+
+//===----------------------------------------------------------------------===//
 // Test Ops with effects
 //===----------------------------------------------------------------------===//
 
 def TestResource : Resource<"TestResource">;
 
 def TestEffectsOpA : TEST_Op<"op_with_effects_a"> {
   let arguments = (ins
     Arg<Variadic<AnyMemRef>, "", [MemRead]>,

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

 
 def DOp : NS_Op<"d_op", []> {
   let arguments = (ins
     I32Attr:$i32_attr,
     F64Attr:$f64_attr,
     StrAttr:$str_attr,
     BoolAttr:$bool_attr,
     SomeI32Enum:$enum_attr,
-    DefaultValuedAttr<I32Attr, "42">:$dv_i32_attr,
+    DefaultValuedAttr<I32Attr, "42", 1>:$dv_i32_attr,
     DefaultValuedAttr<F64Attr, "8.">:$dv_f64_attr,
     DefaultValuedStrAttr<StrAttr, "abc">:$dv_str_attr,
     DefaultValuedAttr<BoolAttr, "true">:$dv_bool_attr,
     DefaultValuedAttr<SomeI32Enum, "::SomeI32Enum::case5">:$dv_enum_attr
   );
 }
 
 // DECL-LABEL: DOp declarations

mlir/test/mlir-tblgen/op-format.mlir

func.func @else_anchor_op(%a: !test.else_anchor<?>, %b: !test.else_anchor<5>) {
   // CHECK: test.else_anchor(?) {a = !test.else_anchor_struct<?>}
   test.else_anchor(?) {a = !test.else_anchor_struct<?>}
   // CHECK: test.else_anchor(%{{.*}} : !test.else_anchor<?>) {a = !test.else_anchor_struct<a = 0>}
   test.else_anchor(%a : !test.else_anchor<?>) {a = !test.else_anchor_struct<a = 0>}
   // CHECK: test.else_anchor(%{{.*}} : !test.else_anchor<5>) {a = !test.else_anchor_struct<b = 0>}
   test.else_anchor(%b : !test.else_anchor<5>) {a = !test.else_anchor_struct<b = 0>}
   return
 }
+
+//===----------------------------------------------------------------------===//
+// Check DefaultValuedAttr Printing
+//===----------------------------------------------------------------------===//

mlir/tools/mlir-tblgen/OpFormatGen.cpp

const char *enumAttrBeginPrinterCode = R"(
   {
     auto caseValue = {0}();
     auto caseValueStr = {1}(caseValue);
 )";
 
 /// Generate the printer for the 'attr-dict' directive.
 static void genAttrDictPrinter(OperationFormat &fmt, Operator &op,
                                MethodBody &body, bool withKeyword) {
-  body << "  _odsPrinter.printOptionalAttrDict"
-       << (withKeyword ? "WithKeyword" : "")
-       << "((*this)->getAttrs(), /*elidedAttrs=*/{";
+  body << "  ::llvm::SmallVector<::llvm::StringRef> elidedAttrs;\n";
   // Elide the variadic segment size attributes if necessary.
   if (!fmt.allOperands &&
       op.getTrait("::mlir::OpTrait::AttrSizedOperandSegments"))
-    body << "\"operand_segment_sizes\", ";
+    body << "  elidedAttrs.push_back(\"operand_segment_sizes\");\n";
   if (!fmt.allResultTypes &&
       op.getTrait("::mlir::OpTrait::AttrSizedResultSegments"))
-    body << "\"result_segment_sizes\", ";
-  if (!fmt.inferredAttributes.empty()) {
-    for (const auto &attr : fmt.inferredAttributes)
-      body << "\"" << attr.getKey() << "\", ";
+    body << "  elidedAttrs.push_back(\"result_segment_sizes\");\n";
+    body << "  elidedAttrs.push_back(\"" << attr.getKey() << "\");\n";
+  for (const auto &attr : fmt.usedAttributes)
+    body << "  elidedAttrs.push_back(\"" << attr->name << "\");\n";
+  // Add code to check attributes for equality with the default value
+  // for attributes with the elidePrintingDefaultValue bit set.
+  for (const auto &namedAttr : op.getAttributes()) {
+    const auto &attr = namedAttr.attr;
+    if (!attr.isDerivedAttr() && attr.hasDefaultValue() &&
+        attr.elidePrintingDefaultValue()) {
+      const auto &name = namedAttr.name;
+      FmtContext fctx;
+      fctx.withBuilder("odsBuilder");
+      std::string defaultValue = std::string(
+          tgfmt(attr.getConstBuilderTemplate(), &fctx, attr.getDefaultValue()));
+      body << "  {\n";
+      body << "     ::mlir::Builder odsBuilder(getContext());\n";
+      body << "     auto attr = " << op.getGetterName(name) << "Attr();\n";
+      body << "     if(attr && (attr == " << defaultValue << "))\n";
+      body << "       elidedAttrs.push_back(\"" << name << "\");\n";
+      body << "  }\n";
     }
-  llvm::interleaveComma(
-      fmt.usedAttributes, body,
-      [&](const NamedAttribute *attr) { body << "\"" << attr->name << "\""; });
-  body << "});\n";
+  }
+  body << "  _odsPrinter.printOptionalAttrDict"
+       << (withKeyword ? "WithKeyword" : "")
+       << "((*this)->getAttrs(), elidedAttrs);\n";
 }
 
 /// Generate the printer for a literal value. `shouldEmitSpace` is true if a
 /// space should be emitted before this element. `lastWasPunctuation` is true if
 /// the previous element was a punctuation literal.
 static void genLiteralPrinter(StringRef value, MethodBody &body,
                               bool &shouldEmitSpace, bool &lastWasPunctuation) {
   body << "  _odsPrinter";