[mlir][OpFormatGen] Update "custom" directives for attributes.

[mikeurbach]

Summary

This tweaks the generated code for parsing attributes with a custom
directive to call addAttribute on the OperationState directly,
and adds a newline after this call. Previously, the generated code
would call addAttribute on the OperationState field attributes,
which has no such method and fails to compile. Furthermore, the lack
of newline would generate code with incorrectly formatted single line
if statements. Added tests for parsing and printing attributes with
a custom directive.

[mehdi_amini]

LGTM

It wasn't clear from the description that this is actually fixing something that wasn't working, can you update the description in the commit?

[mikeurbach]

Update description about custom directive attribute parsing

This adds some description about the issue and why this revision was needed.

[mikeurbach]

@mehdi_amini I've amended the commit description to include some background on the issue this revision intends to fix.

[mikeurbach]

Use arc diff --edit --verbatim to update summary

[mikeurbach]

This is my first contribution to LLVM, so I will need need some assistance per the policy. @mehdi_amini as the reviewer, do you mind committing this on my behalf? Not sure if this is already associated in Phabricator, but my name is Mike Urbach, and email is [e-mail]. The GitHub username is also mikeurbach, if that is needed.

mlir/test/lib/Dialect/Test/TestDialect.cpp

parseCustomDirectiveSuccessors(OpAsmParser &parser, Block *&successor,
   if (failed(parser.parseOptionalComma()))
     return success();
   Block *varSuccessor;
   if (parser.parseSuccessor(varSuccessor))
     return failure();
   varSuccessors.append(2, varSuccessor);
   return success();
 }
+static ParseResult parseCustomDirectiveAttributes(OpAsmParser &parser,
+                                                  IntegerAttr &attr,
+                                                  IntegerAttr &optAttr) {
+  if (parser.parseAttribute(attr))
+    return failure();
+  if (succeeded(parser.parseOptionalComma())) {
+    if (parser.parseAttribute(optAttr))
+      return failure();
+  }
+  return success();
+}
 
 //===----------------------------------------------------------------------===//
 // Printing
 
 static void printCustomDirectiveOperands(OpAsmPrinter &printer, Value operand,
                                          Value optOperand,
                                          OperandRange varOperands) {
   printer << operand;

 }
 static void printCustomDirectiveSuccessors(OpAsmPrinter &printer,
                                            Block *successor,
                                            SuccessorRange varSuccessors) {
   printer << successor;
   if (!varSuccessors.empty())
     printer << ", " << varSuccessors.front();
 }
+static void printCustomDirectiveAttributes(OpAsmPrinter &printer,
+                                           Attribute attribute,
+                                           Attribute optAttribute) {
+  printer << attribute;
+  if (optAttribute)
+    printer << ", " << optAttribute;
+}
 
 //===----------------------------------------------------------------------===//
 // Test IsolatedRegionOp - parse passthrough region arguments.
 //===----------------------------------------------------------------------===//
 
 static ParseResult parseIsolatedRegionOp(OpAsmParser &parser,
                                          OperationState &result) {
   OpAsmParser::OperandType argInfo;

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

def FormatCustomDirectiveSuccessors
   let assemblyFormat = [{
     custom<CustomDirectiveSuccessors>(
       $successor, $successors
     )
     attr-dict
   }];
 }
 
+def FormatCustomDirectiveAttributes
+    : TEST_Op<"format_custom_directive_attributes"> {
+  let arguments = (ins I64Attr:$attr, OptionalAttr<I64Attr>:$optAttr);
+  let assemblyFormat = [{
+    custom<CustomDirectiveAttributes>(
+      $attr, $optAttr
+    )
+    attr-dict
+  }];
+}
+
 //===----------------------------------------------------------------------===//
 // AllTypesMatch type inference
 
 def FormatAllTypesMatchVarOp : TEST_Op<"format_all_types_match_var", [
     AllTypesMatch<["value1", "value2", "result"]>
   ]> {
   let arguments = (ins AnyType:$value1, AnyType:$value2);
   let results = (outs AnyType:$result);

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

 test.format_custom_directive_operands %i64 -> (%i64)
 
 // CHECK: test.format_custom_directive_operands_and_types %[[I64]], %[[I64]] -> (%[[I64]]) : i64, i64 -> (i64)
 test.format_custom_directive_operands_and_types %i64, %i64 -> (%i64) : i64, i64 -> (i64)
 
 // CHECK: test.format_custom_directive_operands_and_types %[[I64]] -> (%[[I64]]) : i64 -> (i64)
 test.format_custom_directive_operands_and_types %i64 -> (%i64) : i64 -> (i64)
 
+// CHECK: test.format_custom_directive_attributes 54 : i64
+test.format_custom_directive_attributes 54 : i64
+
+// CHECK: test.format_custom_directive_attributes 54 : i64, 46 : i64
+test.format_custom_directive_attributes 54 : i64, 46 : i64
+
 // CHECK: test.format_custom_directive_regions {
 // CHECK-NEXT: test.return
 // CHECK-NEXT: }
 test.format_custom_directive_regions {
   "test.return"() : () -> ()
 }
 
 // CHECK: test.format_custom_directive_regions {

mlir/tools/mlir-tblgen/OpFormatGen.cpp

static void genCustomDirectiveParser(CustomDirective *dir, OpMethodBody &body) {
 
   // After parsing, add handling for any of the optional constructs.
   for (Element &param : dir->getArguments()) {
     if (auto *attr = dyn_cast<AttributeVariable>(&param)) {
       const NamedAttribute *var = attr->getVar();
       if (var->attr.isOptional())
         body << llvm::formatv("    if ({0}Attr)\n  ", var->name);
 
-      body << llvm::formatv(
-          "    result.attributes.addAttribute(\"{0}\", {0}Attr);", var->name);
+      body << llvm::formatv("    result.addAttribute(\"{0}\", {0}Attr);\n",
+                            var->name);
     } else if (auto *operand = dyn_cast<OperandVariable>(&param)) {
       const NamedTypeConstraint *var = operand->getVar();
       if (!var->isOptional())
         continue;
       body << llvm::formatv("    if ({0}Operand.hasValue())\n"
                             "      {0}Operands.push_back(*{0}Operand);\n",
                             var->name);
     } else if (auto *dir = dyn_cast<TypeRefDirective>(&param)) {