[mlir] Support optional attributes in assembly formats

[gysit]

Summary

This revision adds support for assembly formats with optional attributes. It elides optional attributes that are part of the syntax from the attribute dictionary.

[Kayjukh]

LGTM after fixing the use of curly braces.

mlir/tools/mlir-tblgen/OpFormatGen.cpp

893

nit: It could be advisable to use some curly braces for such nested statements.

[ftynse]

mlir/tools/mlir-tblgen/OpFormatGen.cpp

893

MLIR is actually quite adamant on the opposite

[Kayjukh]

mlir/tools/mlir-tblgen/OpFormatGen.cpp

893

Interesting. Is there a mention of this in the LLVM coding standards? If not I guess it is mostly up to preference.

[ftynse]

mlir/tools/mlir-tblgen/OpFormatGen.cpp

893

It's not in LLVM standards or MLIR fixups to them. In practice, MLIR is aggressive on eliding the braces for single-line one-statement bodies.

[Kayjukh]

mlir/tools/mlir-tblgen/OpFormatGen.cpp

893

Sure, for single-line bodies I totally agree that avoiding the braces can make things more readable. What I was pointing out in the first comment was

if (...)
  for (...)
    if (...)
      // ...

which I find less readable and potentially more error-prone than

if (...) {
  for (...) {
    if (...)
      // ...
  }
}

[mehdi_amini]

mlir/tools/mlir-tblgen/OpFormatGen.cpp

893

If there is a single statement body inside the if, we'll go without braces in MLIR in general I believe

[rriddle]

mlir/tools/mlir-tblgen/OpFormatGen.cpp

893

This is the style we follow. I generally err on the side of removing "trivial" braces(i.e. 1-2 lines). When they start to span multiple lines it becomes unreadable IMO. @Kayjukh I think your example highlights the style nicely, though in many situations i would also elide the braces around the for.

for (...)
  if (...)
     // ...

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

 }
 
 // Test that we elide attributes that are within the syntax.
 def FormatAttrOp : TEST_Op<"format_attr_op"> {
   let arguments = (ins I64Attr:$attr);
   let assemblyFormat = "$attr attr-dict";
 }
 
+// Test that we elide optional attributes that are within the syntax.
+def FormatOptAttrOp : TEST_Op<"format_opt_attr_op"> {
+  let arguments = (ins OptionalAttr<I64Attr>:$opt_attr);
+  let assemblyFormat = "(`(`$opt_attr^`)`)? attr-dict";
+}
+
 // Test that we elide attributes that are within the syntax.
 def FormatAttrDictWithKeywordOp : TEST_Op<"format_attr_dict_w_keyword"> {
-  let arguments = (ins I64Attr:$attr);
+  let arguments = (ins I64Attr:$attr, OptionalAttr<I64Attr>:$opt_attr);
   let assemblyFormat = "attr-dict-with-keyword";
 }
 
 // Test that we don't need to provide types in the format if they are buildable.
 def FormatBuildableTypeOp : TEST_Op<"format_buildable_type_op"> {
   let arguments = (ins I64:$buildable);
   let results = (outs I64:$buildable_res);
   let assemblyFormat = "$buildable attr-dict";

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

 // RUN: mlir-opt -allow-unregistered-dialect %s | mlir-opt -allow-unregistered-dialect -verify-diagnostics | FileCheck %s
 
 // CHECK: %[[I64:.*]] =
 %i64 = "foo.op"() : () -> (i64)
 // CHECK: %[[MEMREF:.*]] =
 %memref = "foo.op"() : () -> (memref<1xf64>)
 
 // CHECK: test.format_literal_op keyword_$. -> :, = <> () [] {foo.some_attr}
 test.format_literal_op keyword_$. -> :, = <> () [] {foo.some_attr}
 
 // CHECK: test.format_attr_op 10
 // CHECK-NOT: {attr
 test.format_attr_op 10
 
+// CHECK: test.format_opt_attr_op(10)
+// CHECK-NOT: {opt_attr
+test.format_opt_attr_op(10)
+
 // CHECK: test.format_attr_dict_w_keyword attributes {attr = 10 : i64}
 test.format_attr_dict_w_keyword attributes {attr = 10 : i64}
 
+// CHECK: test.format_attr_dict_w_keyword attributes {attr = 10 : i64, opt_attr = 10 : i64}
+test.format_attr_dict_w_keyword attributes {attr = 10 : i64, opt_attr = 10 : i64}
+
 // CHECK: test.format_buildable_type_op %[[I64]]
 %ignored = test.format_buildable_type_op %i64
 
 //===----------------------------------------------------------------------===//
 // Format results
 //===----------------------------------------------------------------------===//
 
 // CHECK: test.format_result_a_op memref<1xf64>

mlir/tools/mlir-tblgen/OpFormatGen.cpp

 //===----------------------------------------------------------------------===//
 // PrinterGen
 
 /// Generate the printer for the 'attr-dict' directive.
 static void genAttrDictPrinter(OperationFormat &fmt, Operator &op,
                                OpMethodBody &body, bool withKeyword) {
   // Collect all of the attributes used in the format, these will be elided.
   SmallVector<const NamedAttribute *, 1> usedAttributes;
-  for (auto &it : fmt.elements)
+  for (auto &it : fmt.elements) {
     if (auto *attr = dyn_cast<AttributeVariable>(it.get()))
       usedAttributes.push_back(attr->getVar());
+    // Collect the optional attributes.
+    if (auto *opt = dyn_cast<OptionalElement>(it.get())) {
+      for (auto &elem : opt->getElements()) {
+        if (auto *attr = dyn_cast<AttributeVariable>(&elem))
+          usedAttributes.push_back(attr->getVar());
+      }
+    }
+  }
 
   body << "  p.printOptionalAttrDict" << (withKeyword ? "WithKeyword" : "")
        << "(getAttrs(), /*elidedAttrs=*/{";
   // Elide the variadic segment size attributes if necessary.
   if (!fmt.allOperands && op.getTrait("OpTrait::AttrSizedOperandSegments"))
     body << "\"operand_segment_sizes\", ";
   llvm::interleaveComma(usedAttributes, body, [&](const NamedAttribute *attr) {
     body << "\"" << attr->name << "\"";