[MLIR] Change ODS collective params build method to provide an empty default value for named attributes

[jurahul]

Summary

• Provide default value for ArrayRef<NamedAttribute> attributes parameter of the collective params build method.
• Change the genSeparateArgParamBuilder function to not generate build methods that may be ambiguous with the new collective params build method.
• This change should help eliminate passing empty NamedAttribue ArrayRef when the collective params build method is used

[jpienaar]

Could you add some unit tests?

[jurahul]

I looked into extending the op-decl.td unit test, but the way the tests are currently setup we have a mix of mlir and td tests under test/mlir-tblgen, so its not possible to observe the unit test failure because mlir-opt fails to build with a broken mlir-tblgen. In an attempt to make the mlir-tblgen dependent tests to be run independently from mlit-opt dependent tests, I have put up another review (https://reviews.llvm.org/D83528) to split the tests under mlir-tblgen into two directories. I'll update the this one with a unit tests once that change goes in and I can test (easily) that my unit tests fails if I disable the suppression of generating the ambiguous build methods

[jurahul]

Add unit test

[jpienaar]

So the goal of this change is to avoid needing to specify empty array of attributes if there are none for the collective case? (We have default attributes and so the description was a bit ambiguous wrt that)

In D83517#2143121, @jurahul wrote:

so its not possible to observe the unit test failure because mlir-opt fails to build with a broken mlir-tblgen.

Yes, but these unit tests don't test if mlir-tblgen is broken, they test if it produces expected results.

Now if mlir-tblgen is incorrect to the point where mlir-tblgen produces invalid C++ code so that mlir-opt can't compile, then yes we won't see the unit tests mlir-tblgen running. That's a pain and one affecting folks working on mlir-tblgen. Today they could run the test commands manually or just disable also building mlir-opt in the test suite and run the tests, buts it not automatic.

So if we look to improve that, what would the goal be to make it easier for folks developing mlir-tblgen? Or to test valid C++ code is generated? E.g., we don't test the latter except with mlir-opt today and so is the plan to address that? That is also a different discussion and we could discuss on the splitting change to

[jurahul]

So the goal of this change is to avoid needing to specify empty array of attributes if there are none for the collective case? (We have default attributes and so the description was a bit ambiguous wrt that)

Yes, that's the goal. Currently I see several uses of this build/create method where we are passing empty array of attributes and the goal is to eliminate that from the code. This has nothing to do with default *values* attributes, I think I saw another bug regarding them, bit this change is not attempting to fix that. I can reword this to "Change collective params build method to have an empty default value for attributes".

As for the unit test issue, yes, lets have that as a separate discussion. For now, am maintaining the status quo there and extending the existing test with addition tests. If/when we migrate the mlir-tblgen unit test to the C++ unittest, I expect these will migrate with that.

[mehdi_amini]

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

959

Typo: "collective pramas"

973

Typo: ambiguious -> ambiguous (and same typo below)

(thanks for writing a good doc!)

[jurahul]

Fix spelling in comment

[jpienaar]

Nice, thanks for expanding the documentation there

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

186

suppression ?

Could you expand to say why?

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

965

Missing )

971

Why is a single variadic result required? I think you might mean at most one variadic result

1227

s/if/as/ ? (if here makes me think there are cases where we get here where this is not true)

[jurahul]

More spellcheck..

[jurahul]

Address review comments

[jpienaar]

Nice, thanks

[rriddle]

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

1227

nit: Please move the comment out of the else body, or add braces. This makes the body "non-trivial".

[jurahul]

Update comment to describe the conditions more clearly

[jurahul]

Braces for else case

[rriddle]

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

1227

nit: When adding braces to an else/if, add the braces to all of the if/elses in the chain.

[jurahul]

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

971

I do mean a single variadic result. The build() method we are trying to suppress is one with a single ArrayReg<Type> argument for result types and a single ArrayRef<Value> for operands, and no other parameters after that. That condition for the first one is either a a collective paramKind for result type, or a separate with a single variadic result. If there are no results, I am assuming the separate paramKind will not generate any ArrayRef<Type> argument for a non-result (buildParamList case TypeParamKind::Separate iterates over numResults, which will be 0).

I guess I could make the comment clearer by segregating these 2 criteria and describing the conditions for these 2 differently (conditions for generating a single ArrrayRef<Type> and condition for generating a single ArrayRef<Value> for operands).

[jurahul]

And braces for if

mlir/lib/Conversion/StandardToSPIRV/ConvertStandardToSPIRV.cpp

matchAndRewrite(StdOp operation, ArrayRef<Value> operands,
     assert(operands.size() <= 2);
     auto dstType = this->typeConverter.convertType(operation.getType());
     if (!dstType)
       return failure();
     if (isUnsignedOp<SPIRVOp>() && dstType != operation.getType()) {
       return operation.emitError(
           "bitwidth emulation is not implemented yet on unsigned op");
     }
-    rewriter.template replaceOpWithNewOp<SPIRVOp>(operation, dstType, operands,
-                                                  ArrayRef<NamedAttribute>());
+    rewriter.template replaceOpWithNewOp<SPIRVOp>(operation, dstType, operands);
     return success();
   }
 };
 
 /// Converts std.remi_signed to SPIR-V ops.
 ///
 /// This cannot be merged into the template unary/binary pattern due to
 /// Vulkan restrictions over spv.SRem and spv.SMod.
public:
   matchAndRewrite(StdOp operation, ArrayRef<Value> operands,
                   ConversionPatternRewriter &rewriter) const override {
     assert(operands.size() == 2);
     auto dstType =
         this->typeConverter.convertType(operation.getResult().getType());
     if (!dstType)
       return failure();
     if (isBoolScalarOrVector(operands.front().getType())) {
-      rewriter.template replaceOpWithNewOp<SPIRVLogicalOp>(
-          operation, dstType, operands, ArrayRef<NamedAttribute>());
+      rewriter.template replaceOpWithNewOp<SPIRVLogicalOp>(operation, dstType,
+                                                           operands);
     } else {
-      rewriter.template replaceOpWithNewOp<SPIRVBitwiseOp>(
-          operation, dstType, operands, ArrayRef<NamedAttribute>());
+      rewriter.template replaceOpWithNewOp<SPIRVBitwiseOp>(operation, dstType,
+                                                           operands);
     }
     return success();
   }
 };
 
 /// Converts composite std.constant operation to spv.constant.
 class ConstantCompositeOpPattern final : public SPIRVOpLowering<ConstantOp> {
 public:
if (srcType.isSignlessInteger() && srcType.getIntOrFloatBitWidth() == 1)
       return failure();
     auto dstType =
         this->typeConverter.convertType(operation.getResult().getType());
     if (dstType == srcType) {
       // Due to type conversion, we are seeing the same source and target type.
       // Then we can just erase this operation by forwarding its operand.
       rewriter.replaceOp(operation, operands.front());
     } else {
-      rewriter.template replaceOpWithNewOp<SPIRVOp>(
-          operation, dstType, operands, ArrayRef<NamedAttribute>());
+      rewriter.template replaceOpWithNewOp<SPIRVOp>(operation, dstType,
+                                                    operands);
     }
     return success();
   }
 };
 
 /// Converts std.xor to SPIR-V operations.
 class XOrOpPattern final : public SPIRVOpLowering<XOrOp> {
 public:
XOrOpPattern::matchAndRewrite(XOrOp xorOp, ArrayRef<Value> operands,
   assert(operands.size() == 2);
 
   if (isBoolScalarOrVector(operands.front().getType()))
     return failure();
 
   auto dstType = typeConverter.convertType(xorOp.getType());
   if (!dstType)
     return failure();
-  rewriter.replaceOpWithNewOp<spirv::BitwiseXorOp>(xorOp, dstType, operands,
-                                                   ArrayRef<NamedAttribute>());
+  rewriter.replaceOpWithNewOp<spirv::BitwiseXorOp>(xorOp, dstType, operands);
 
   return success();
 }
 
 //===----------------------------------------------------------------------===//
 // Pattern population
 //===----------------------------------------------------------------------===//
 

mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp

Value Importer::processConstant(llvm::Constant *c) {
   if (auto *cn = dyn_cast<llvm::ConstantPointerNull>(c)) {
     LLVMType type = processType(cn->getType());
     if (!type)
       return nullptr;
     return instMap[c] = bEntry.create<NullOp>(unknownLoc, type);
   }
   if (auto *GV = dyn_cast<llvm::GlobalVariable>(c))
     return bEntry.create<AddressOfOp>(UnknownLoc::get(context),
-                                      processGlobal(GV),
-                                      ArrayRef<NamedAttribute>());
+                                      processGlobal(GV));
 
   if (auto *ce = dyn_cast<llvm::ConstantExpr>(c)) {
     llvm::Instruction *i = ce->getAsInstruction();
     OpBuilder::InsertionGuard guard(b);
     b.setInsertionPoint(currentEntryBlock, currentEntryBlock->begin());
     if (failed(processInstruction(i)))
       return nullptr;
     assert(instMap.count(i));
case llvm::Instruction::Call: {
     if (llvm::Function *callee = ci->getCalledFunction()) {
       op = b.create<CallOp>(loc, tys, b.getSymbolRefAttr(callee->getName()),
                             ops);
     } else {
       Value calledValue = processValue(ci->getCalledOperand());
       if (!calledValue)
         return failure();
       ops.insert(ops.begin(), calledValue);
-      op = b.create<CallOp>(loc, tys, ops, ArrayRef<NamedAttribute>());
+      op = b.create<CallOp>(loc, tys, ops);
     }
     if (!ci->getType()->isVoidTy())
       v = op->getResult(0);
     return success();
   }
   case llvm::Instruction::LandingPad: {
     llvm::LandingPadInst *lpi = cast<llvm::LandingPadInst>(inst);
     SmallVector<Value, 4> ops;
for (auto *op : gep->operand_values()) {
       Value value = processValue(op);
       if (!value)
         return failure();
       ops.push_back(value);
     }
     Type type = processType(inst->getType());
     if (!type)
       return failure();
-    v = b.create<GEPOp>(loc, type, ops, ArrayRef<NamedAttribute>());
+    v = b.create<GEPOp>(loc, type, ops);
     return success();
   }
   }
 }
 
 FlatSymbolRefAttr Importer::getPersonalityAsAttr(llvm::Function *f) {
   if (!f->hasPersonalityFn())
     return nullptr;

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

 def NS_GOp : NS_Op<"op_with_fixed_return_type", []> {
   let arguments = (ins AnyType:$a);
   let results = (outs I32:$b);
 }
 
 // CHECK-LABEL: class GOp :
 // CHECK: static ::mlir::LogicalResult inferReturnTypes
 
+// Check default value for collective params builder. Check that other builders
+// are generated as well.
+def NS_HCollectiveParamsOp : NS_Op<"op_collective_params", []> {
+  let arguments = (ins AnyType:$a);
+  let results = (outs AnyType:$b);
+}
+
+// CHECK_LABEL: class NS_HCollectiveParamsOp :
+// CHECK: static void build(::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &odsState, ::mlir::Type b, ::mlir::Value a);
+// CHECK: static void build(::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &odsState, ::llvm::ArrayRef<::mlir::Type> resultTypes, ::mlir::Value a);
+// CHECK: static void build(::mlir::OpBuilder &, ::mlir::OperationState &odsState, ::llvm::ArrayRef<::mlir::Type> resultTypes, ::mlir::ValueRange operands, ::llvm::ArrayRef<::mlir::NamedAttribute> attributes = {})
+
+// Check suppression of "separate arg, separate result" build method for an op
+// with single variadic arg and single variadic result (since it will be
+// ambiguous with the collective params build method).
+def NS_HCollectiveParamsSuppress0Op : NS_Op<"op_collective_suppress0", []> {
+  let arguments = (ins Variadic<I32>:$a);
+  let results = (outs Variadic<I32>:$b);
+}
+
+// CHECK_LABEL: class NS_HCollectiveParamsSuppress0Op :
+// CHECK-NOT: static void build(::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &odsState, ::llvm::ArrayRef<::mlir::Type> b, ::mlir::ValueRange a);
+// CHECK: static void build(::mlir::OpBuilder &, ::mlir::OperationState &odsState, ::llvm::ArrayRef<::mlir::Type> resultTypes, ::mlir::ValueRange operands, ::llvm::ArrayRef<::mlir::NamedAttribute> attributes = {});
+
+// Check suppression of "separate arg, collective result" build method for an op
+// with single variadic arg and non variadic result (since it will be
+// ambiguous with the collective params build method).
+def NS_HCollectiveParamsSuppress1Op : NS_Op<"op_collective_suppress1", []> {
+  let arguments = (ins Variadic<I32>:$a);
+  let results = (outs I32:$b);
+}
+
+// CHECK_LABEL: class NS_HCollectiveParamsSuppress1Op :
+// CHECK-NOT: static void build(::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &odsState, ::llvm::ArrayRef<::mlir::Type> b, ::mlir::ValueRange a);
+// CHECK: static void build(::mlir::OpBuilder &, ::mlir::OperationState &odsState, ::llvm::ArrayRef<::mlir::Type> resultTypes, ::mlir::ValueRange operands, ::llvm::ArrayRef<::mlir::NamedAttribute> attributes = {});
+
+// Check suppression of "separate arg, collective result" build method for an op
+// with single variadic arg and > 1 variadic result (since it will be
+// ambiguous with the collective params build method). Note that "separate arg,
+// separate result" build method should be generated in this case as its not
+// ambiguous with the collective params build method.
+def NS_HCollectiveParamsSuppress2Op : NS_Op<"op_collective_suppress2", [SameVariadicResultSize]> {
+  let arguments = (ins Variadic<I32>:$a);
+  let results = (outs Variadic<I32>:$b, Variadic<F32>:$c);
+}
+// CHECK_LABEL: class NS_HCollectiveParamsSuppress2Op :
+// CHECK: static void build(::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &odsState, ::llvm::ArrayRef<::mlir::Type> b, ::llvm::ArrayRef<::mlir::Type> c, ::mlir::ValueRange a);
+// CHECK-NOT: static void build(::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &odsState, ::llvm::ArrayRef<::mlir::Type> b, ::mlir::ValueRange a);
+// CHECK: static void build(::mlir::OpBuilder &, ::mlir::OperationState &odsState, ::llvm::ArrayRef<::mlir::Type> resultTypes, ::mlir::ValueRange operands, ::llvm::ArrayRef<::mlir::NamedAttribute> attributes = {});
+
 // Check that default builders can be suppressed.
 // ---
 
 def NS_SkipDefaultBuildersOp : NS_Op<"skip_default_builders", []> {
   let skipDefaultBuilders = 1;
   let builders = [OpBuilder<"Value val">];
 }
 

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

case TypeParamKind::Collective:
            << (op.getNumResults() - op.getNumVariableLengthResults())
            << "u && \"mismatched number of results\");\n";
       body << "  " << builderOpState << ".addTypes(resultTypes);\n";
       return;
     }
     llvm_unreachable("unhandled TypeParamKind");
   };
 
+  // A separate arg param builder method will have a signature which is
+  // ambiguous with the collective params build method (generated in
+  // `genCollectiveParamBuilder` function below) if it has a single
+  // `ArrayReg<Type>` parameter for result types and a single `ArrayRef<Value>`
+  // parameter for the operands, no parameters after that, and the collective
+  // params build method has `attributes` as its last parameter (with
+  // a default value). This will happen when all of the following are true:
+  // 1. [`attributes` as last parameter in collective params build method]:
+  //    getNumVariadicRegions must be 0 (otherwise the collective params build
+  //    method ends with a `numRegions` param, and we don't specify default
+  //    value for attributes).
+  // 2. [single `ArrayRef<Value>` parameter for operands, and no parameters
+  //    after that]: numArgs() must be 1 (if not, each arg gets a separate param
+  //    in the build methods generated here) and the single arg must be a
+  //    non-attribute variadic argument.
+  // 3. [single `ArrayReg<Type>` parameter for result types]:
+  //      3a. paramKind should be Collective, or
+  //      3b. paramKind should be Separate and there should be a single variadic
+  //          result
+  //
+  // In that case, skip generating such ambiguous build methods here.
+  bool hasSingleVariadicResult =
+      op.getNumResults() == 1 && op.getResult(0).isVariadic();
+
+  bool hasSingleVariadicArg =
+      op.getNumArgs() == 1 &&
+      op.getArg(0).is<tblgen::NamedTypeConstraint *>() &&
+      op.getOperand(0).isVariadic();
+  bool hasNoVariadicRegions = op.getNumVariadicRegions() == 0;
+
   for (auto attrType : attrBuilderType) {
+    // Case 3b above.
+    if (!(hasNoVariadicRegions && hasSingleVariadicArg &&
+          hasSingleVariadicResult))
       emit(attrType, TypeParamKind::Separate, /*inferType=*/false);
     if (canInferType(op))
       emit(attrType, TypeParamKind::None, /*inferType=*/true);
-    // Emit separate arg build with collective type, unless there is only one
-    // variadic result, in which case the above would have already generated
-    // the same build method.
-    if (!(op.getNumResults() == 1 && op.getResult(0).isVariableLength()))
+    // The separate arg + collective param kind method will be:
+    // (a) Same as the separate arg + separate param kind method if there is
+    //     only one variadic result.
+    // (b) Ambiguous with the collective params method under conditions in (3a)
+    //     above.
+    // In either case, skip generating such build method.
+    if (!hasSingleVariadicResult &&
+        !(hasNoVariadicRegions && hasSingleVariadicArg))
       emit(attrType, TypeParamKind::Collective, /*inferType=*/false);
   }
 }
 
 void OpEmitter::genUseOperandAsResultTypeCollectiveParamBuilder() {
   // If this op has a variadic result, we cannot generate this builder because
   // we don't know how many results to create.
   if (op.getNumVariableLengthResults() != 0)
void OpEmitter::genCollectiveParamBuilder() {
   int numNonVariadicOperands = numOperands - numVariadicOperands;
   // Signature
   std::string params =
       std::string("::mlir::OpBuilder &, ::mlir::OperationState &") +
       builderOpState +
       ", ::llvm::ArrayRef<::mlir::Type> resultTypes, ::mlir::ValueRange "
       "operands, "
       "::llvm::ArrayRef<::mlir::NamedAttribute> attributes";
-  if (op.getNumVariadicRegions())
+  if (op.getNumVariadicRegions()) {
     params += ", unsigned numRegions";
+  } else {
+    // Provide default value for `attributes` since its the last parameter
+    params += " = {}";
+  }
   auto &m = opClass.newMethod("void", "build", params, OpMethod::MP_Static);
   auto &body = m.body();
 
   // Operands
   if (numVariadicOperands == 0 || numNonVariadicOperands != 0)
     body << "  assert(operands.size()"
          << (numVariadicOperands != 0 ? " >= " : " == ")
          << numNonVariadicOperands