[mlir][assemblyFormat] Fix bug when using AttrSizedOperandSegments trait with only non-buildable operand types

[maerhart]

Summary

When creating an operation with

• AttrSizedOperandSegments trait
• Variadic operands of only non-buildable types
• assemblyFormat to automatically generate the parser

the builder local variable is used, but never declared.
This adds a fix as well as a test for this case as existing ones use buildable types only.

[Kayjukh]

mlir/tools/mlir-tblgen/OpFormatGen.cpp

743–744

This could be removed and the next lines adapted as noted in the comment.

745

We should follow what is done in OperationFormat::genParser and use parser.getBuilder() instead of a potentially fragile local variable declaration.

868

This seems to be a somewhat fragile fix, especially regarding the mirrored declaration of Builder &builder in OperationFormat::genParserTypeResolution. To reduce the coupling between the functions in this file, I would rather suggest to replace the usages of builder with parser.getBuilder().

[maerhart]

Updated the diff to use parser.getBuilder() directly instead of creating a fragile local variable.

[Kayjukh]

nit: The commit message looks a little convoluted. Maybe you could simplify/structure it a little bit.
Otherwise, LGTM.

[rriddle]

Thanks for the fix!

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

def FormatOptionalOperandResultAOp : FormatOptionalOperandResultOpBase<"a", [{
   (`[` $variadic^ `]`)? attr-dict
 }]>;
 
 def FormatOptionalOperandResultBOp : FormatOptionalOperandResultOpBase<"b", [{
   (`(` $optional^ `:` type($optional) `)`)? `:` type($optional_res)
   (`[` $variadic^ `]`)? attr-dict
 }]>;
 
+def FormatTwoVariadicOperandsNoBuildableTypeOp
+    : TEST_Op<"format_two_variadic_operands_no_buildable_type_op",
+              [AttrSizedOperandSegments]> {
+  let arguments = (ins Variadic<AnyType>:$a,
+                       Variadic<AnyType>:$b);
+  let assemblyFormat = [{
+    `(` $a `:` type($a) `)` `->` `(` $b `:` type($b) `)`  attr-dict
+  }];
+}
+
 //===----------------------------------------------------------------------===//
 // Test SideEffects
 //===----------------------------------------------------------------------===//
 
 def SideEffectOp : TEST_Op<"side_effect_op",
     [DeclareOpInterfaceMethods<MemoryEffectsOpInterface>]> {
   let results = (outs AnyType:$result);
 }
 
 #endif // TEST_OPS

mlir/tools/mlir-tblgen/OpFormatGen.cpp

body << "  for (Type type : " << variable->name << "Types) {\n"
          << formatv("      return parser.emitError(parser.getNameLoc()) << "
                     "\"'{0}' must be {1}, but got \" << type;\n",
                     variable->name, constraint.getDescription())
          << "    }\n"
          << "  }\n";
   }
 
   // Initialize the set of buildable types.
   if (!buildableTypes.empty()) {
-    body << "  Builder &builder = parser.getBuilder();\n";
-
     FmtContext typeBuilderCtx;
-    typeBuilderCtx.withBuilder("builder");
+    typeBuilderCtx.withBuilder("parser.getBuilder()");
     for (auto &it : buildableTypes)
       body << "  Type odsBuildableType" << it.second << " = "
            << tgfmt(it.first, &typeBuilderCtx) << ";\n";
   }
 
   // Emit the code necessary for a type resolver.
   auto emitTypeResolver = [&](TypeResolution &resolver, StringRef curVar) {
     if (Optional<int> val = resolver.getBuilderIdx()) {
else
       body << "  result.addSuccessors(" << successor.name << "Successor);\n";
   }
 }
 
 void OperationFormat::genParserVariadicSegmentResolution(Operator &op,
                                                          OpMethodBody &body) {
   if (!allOperands && op.getTrait("OpTrait::AttrSizedOperandSegments")) {
     body << "  result.addAttribute(\"operand_segment_sizes\", "
-         << "builder.getI32VectorAttr({";
+         << "parser.getBuilder().getI32VectorAttr({";
     auto interleaveFn = [&](const NamedTypeConstraint &operand) {
       // If the operand is variadic emit the parsed size.
       if (operand.isVariableLength())
         body << "static_cast<int32_t>(" << operand.name << "Operands.size())";
       else
         body << "1";
     };
     llvm::interleaveComma(op.getOperands(), body, interleaveFn);