[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

958

Typo: "collective pramas"

972

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 ↗

(On Diff #277467)

suppression ?

Could you expand to say why?

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

964

Missing )

970

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

1221

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

1221

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

1221

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

970

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 bitwise standard operations to SPIR-V operations. This is a special
 /// pattern other than the BinaryOpPatternPattern because if the operands are
 /// boolean values, SPIR-V uses different operations (`SPIRVLogicalOp`). For
 /// non-boolean operands, SPIR-V should use `SPIRVBitwiseOp`.
 template <typename StdOp, typename SPIRVLogicalOp, typename SPIRVBitwiseOp>
 class BitwiseOpPattern final : public SPIRVOpLowering<StdOp> {
 public:
   using SPIRVOpLowering<StdOp>::SPIRVOpLowering;
 
   LogicalResult
   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/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
+  // ambiguious with the collective pramas build method (with a default value
+  // for the last `attributes` parameter, generated in
+  // `genCollectiveParamBuilder` function below) if all of the below conditions
+  // are true:
+  // 1. getNumVariadicRegions must be 0 (otherwise the collective params build
+  //    method ends with numRegions param, and we don't specify default value
+  //    for attributes.
+  // 2. 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 variable length
+  //    non-attribute argument.
+  // 3. paramKind should be
+  //      3a. Collective, or
+  //      3b. Separate and there should be a single variadic result
+  //
+  // In that case, skip generating such ambiguious 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) Ambiguious 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() {
   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())
     params += ", unsigned numRegions";
+  else
+    // Provide default value for `attributes` if 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