[mlir] Make UnitAttr's default val in unwrapped builder

[jpienaar]

Summary

UnitAttr is optional but unwrapped builders require it. Make Change onstructing
from bool as required for when not set at moment (for UnitAttr nothing needs to
be constructed, this is true for others here too and can be addressed
together).

Related to #57118

[rriddle]

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

2149

[vzakhari]

FYI, this change breaks Flang build:

In file included from /llvm-project/flang/include/flang/Lower/AbstractConverter.h:21,
                 from /llvm-project/flang/include/flang/Lower/Bridge.h:17,
                 from /llvm-project/flang/lib/Lower/OpenMP.cpp:15:
/llvm-project/mlir/include/mlir/IR/Builders.h: In instantiation of ‘OpTy mlir::OpBuilder::create(mlir::Location, Args&& ...) [with OpTy = mlir::omp::OrderedRegionOp; Args = {std::nullptr_t}]’:
/llvm-project/flang/lib/Lower/OpenMP.cpp:844:42:   required from here
/llvm-project/mlir/include/mlir/IR/Builders.h:460:16: error: call of overloaded ‘build(mlir::OpBuilder&, mlir::OperationState&, std::nullptr_t)’ is ambiguous
  460 |     OpTy::build(*this, state, std::forward<Args>(args)...);
      |     ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /llvm-project/mlir/include/mlir/Dialect/OpenMP/OpenMPDialect.h:33,
                 from /llvm-project/flang/lib/Lower/OpenMP.cpp:25:
/llvm-project/build/tools/mlir/include/mlir/Dialect/OpenMP/OpenMPOps.h.inc:1320:15: note: candidate: ‘static void mlir::omp::OrderedRegionOp::build(mlir::OpBuilder&, mlir::OperationState&, mlir::UnitAttr)’
 1320 |   static void build(::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &odsState, /*optional*/::mlir::UnitAttr simd);
      |               ^~~~~
/llvm-project/build/tools/mlir/include/mlir/Dialect/OpenMP/OpenMPOps.h.inc:1323:15: note: candidate: ‘static void mlir::omp::OrderedRegionOp::build(mlir::OpBuilder&, mlir::OperationState&, mlir::TypeRange, bool)’
 1323 |   static void build(::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &odsState, ::mlir::TypeRange resultTypes, /*optional*/bool simd = false);
      |               ^~~~~

[vzakhari]

In D135058#3837889, @vzakhari wrote:

FYI, this change breaks Flang build:

Fixed build failure in D135308.

mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorOps.td

string description = [{
     ```mlir
     sparse_tensor.sort stable %n, %x1, %x2 jointly y1, %y2
       : memref<?xindex>, memref<?xindex> jointly memref<?xindex>, memref<?xf32>
     ```
   }];
   let assemblyFormat = "(`stable` $stable^)? $n"
                        "`,`$xs (`jointly` $ys^)? attr-dict"
                        "`:` type($xs) (`jointly` type($ys)^)?";
-  let builders = [
-        OpBuilder<(ins "Value":$n, "ValueRange":$xs, "ValueRange":$ys)>
-  ];
   let hasVerifier = 1;
 }
 
 //===----------------------------------------------------------------------===//
 // Sparse Tensor Syntax Operations.
 //===----------------------------------------------------------------------===//
 
 def SparseTensor_BinaryOp : SparseTensor_Op<"binary", [NoSideEffect]>,

mlir/include/mlir/IR/OpBase.td

class TypeAttrOf<Type ty>
   let constBuilderCall = "::mlir::TypeAttr::get($0)";
 }
 
 // The mere presence of unit attributes has a meaning.  Therefore, unit
 // attributes are always treated as optional and accessors to them return
 // "true" if the attribute is present and "false" otherwise.
 def UnitAttr : Attr<CPred<"$_self.isa<::mlir::UnitAttr>()">, "unit attribute"> {
   let storageType = [{ ::mlir::UnitAttr }];
-  let constBuilderCall = "$_builder.getUnitAttr()";
+  let constBuilderCall = "(($0) ? $_builder.getUnitAttr() : nullptr)";
   let convertFromStorage = "$_self != nullptr";
   let returnType = "bool";
+  let defaultValue = "false";
   let valueType = NoneType;
   let isOptional = 1;
 }
 
 //===----------------------------------------------------------------------===//
 // Composite attribute kinds
 
 class DictionaryAttrBase<Pred condition, string summary> :
class ConstantAttr<Attr attribute, string val> : AttrConstraint<
     "constant attribute " # val> {
   Attr attr = attribute;
   string value = val;
 }
 
 class ConstF32Attr<string val> : ConstantAttr<F32Attr, val>;
 def ConstBoolAttrFalse : ConstantAttr<BoolAttr, "false">;
 def ConstBoolAttrTrue : ConstantAttr<BoolAttr, "true">;
-def ConstUnitAttr : ConstantAttr<UnitAttr, "unit">;
+def ConstUnitAttr : ConstantAttr<UnitAttr, "true">;
 
 // Constant string-based attribute. Wraps the desired string in escaped quotes.
 class ConstantStrAttr<Attr attribute, string val>
     : ConstantAttr<attribute, "\"" # val # "\"">;
 
 //===----------------------------------------------------------------------===//
 // Common attribute constraints
 //===----------------------------------------------------------------------===//

mlir/lib/Dialect/SparseTensor/IR/SparseTensorDialect.cpp

LogicalResult SelectOp::verify() {
   regionResult = verifyNumBlockArgs(this, formula, "select",
                                     TypeRange{inputType}, boolType);
   if (failed(regionResult))
     return regionResult;
 
   return success();
 }
 
-void SortOp::build(OpBuilder &odsBuilder, OperationState &odsState, Value n,
-                   ValueRange xs, ValueRange ys) {
-  build(odsBuilder, odsState, n, xs, ys, /*stable=*/false);
-}
-
 LogicalResult SortOp::verify() {
   if (getXs().empty())
     return emitError("need at least one xs buffer.");
 
   auto n = getN().getDefiningOp<arith::ConstantIndexOp>();
 
   Type xtp = getXs().front().getType().cast<MemRefType>().getElementType();
   auto checkTypes = [&](ValueRange operands,

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

 // DEF-LABEL: UnitAttrOp definitions
 // DEF: bool UnitAttrOp::getAttr() {
 // DEF:   return {{.*}} != nullptr
 
 // DEF: ::mlir::Attribute UnitAttrOp::removeAttrAttr() {
 // DEF-NEXT:   (*this)->removeAttr(getAttrAttrName());
 
 // DEF: build(::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &odsState, /*optional*/::mlir::UnitAttr attr)
+// DEF: build(::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &odsState, /*optional*/bool attr)
+
+// DECL-LABEL: UnitAttrOp declarations
+// DECL-NOT: declarations
+// DECL: build(::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &odsState, /*optional*/bool attr = false)
 
 
 // Test elementAttr field of TypedArrayAttr.
 // ---
 
 def SomeTypedArrayAttr : TypedArrayAttrBase<SomeAttr, "SomeAttr array">;
 
 // RECORD-LABEL: def SomeTypedArrayAttr
 // RECORD: Attr elementAttr = SomeAttr;

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

void OpEmitter::genUseOperandAsResultTypeCollectiveParamBuilder() {
 
   // Result types
   SmallVector<std::string, 2> resultTypes(numResults, "operands[0].getType()");
   body << "  " << builderOpState << ".addTypes({"
        << llvm::join(resultTypes, ", ") << "});\n\n";
 }
 
 void OpEmitter::genPopulateDefaultAttributes() {
-  // All done if no attributes have default values.
+  // All done if no attributes, except optional ones, have default values.
   if (llvm::all_of(op.getAttributes(), [](const NamedAttribute &named) {
-        return !named.attr.hasDefaultValue();
+        return !named.attr.hasDefaultValue() || named.attr.isOptional();
       }))
     return;
 
   SmallVector<MethodParameter> paramList;
   paramList.emplace_back("const ::mlir::RegisteredOperationName &", "opName");
   paramList.emplace_back("::mlir::NamedAttrList &", "attributes");
   auto *m = opClass.addStaticMethod("void", "populateDefaultAttrs", paramList);
   ERROR_IF_PRUNED(m, "populateDefaultAttrs", op);
for (const NamedAttribute &namedAttr : op.getAttributes()) {
     if (!attr.hasDefaultValue() || attr.isOptional())
       continue;
     auto index = attrIndex[namedAttr.name];
     body << "if (!attributes.get(attrNames[" << index << "])) {\n";
     FmtContext fctx;
     fctx.withBuilder(odsBuilder);
     std::string defaultValue = std::string(
         tgfmt(attr.getConstBuilderTemplate(), &fctx, attr.getDefaultValue()));
-    body.indent() << formatv(" attributes.append(attrNames[{0}], {1});\n",
-                             index, defaultValue);
+    body.indent() << formatv("attributes.append(attrNames[{0}], {1});\n", index,
+                             defaultValue);
     body.unindent() << "}\n";
   }
 }
 
 void OpEmitter::genInferredTypeCollectiveParamBuilder() {
   SmallVector<MethodParameter> paramList;
   paramList.emplace_back("::mlir::OpBuilder &", "odsBuilder");
   paramList.emplace_back("::mlir::OperationState &", builderOpState);
void OpEmitter::genCodeForAddingArgAndRegionForBuilder(
   }
 
   // Push all attributes to the result.
   for (const auto &namedAttr : op.getAttributes()) {
     auto &attr = namedAttr.attr;
     if (attr.isDerivedAttr() || inferredAttributes.contains(namedAttr.name))
       continue;
 
-    // TODO(jpienaar): The wrapping of optional is different for default or not,
-    // so don't unwrap for default ones that would fail below.
-    bool emitNotNullCheck = (attr.isOptional() && !attr.hasDefaultValue()) ||
-                            (attr.hasDefaultValue() && !isRawValueAttr);
+    // TODO: The wrapping of optional is different for default or not, so don't
+    // unwrap for default ones that would fail below.
+    bool emitNotNullCheck =
+        (attr.isOptional() && !attr.hasDefaultValue()) ||
+        (attr.hasDefaultValue() && !isRawValueAttr) ||
+        // TODO: UnitAttr is optional, not wrapped, but needs to be guarded as
+        // the constant materialization is only for true case.
+        (isRawValueAttr && attr.getAttrDefName() == "UnitAttr");
     if (emitNotNullCheck)
-      body << formatv("  if ({0}) ", namedAttr.name) << "{\n";
+      body.indent() << formatv("if ({0}) ", namedAttr.name) << "{\n";
 
     if (isRawValueAttr && canUseUnwrappedRawValue(attr)) {
       // If this is a raw value, then we need to wrap it in an Attribute
       // instance.
       FmtContext fctx;
       fctx.withBuilder("odsBuilder");
 
       std::string builderTemplate = std::string(attr.getConstBuilderTemplate());
if (isRawValueAttr && canUseUnwrappedRawValue(attr)) {
       body << formatv("  {0}.addAttribute({1}AttrName({0}.name), {2});\n",
                       builderOpState, op.getGetterName(namedAttr.name), value);
     } else {
       body << formatv("  {0}.addAttribute({1}AttrName({0}.name), {2});\n",
                       builderOpState, op.getGetterName(namedAttr.name),
                       namedAttr.name);
     }
     if (emitNotNullCheck)
-      body << "  }\n";
+      body.unindent() << "  }\n";
   }
 
   // Create the correct number of regions.
   for (const NamedRegion &region : op.getRegions()) {
     if (region.isVariadic())
       body << formatv("  for (unsigned i = 0; i < {0}Count; ++i)\n  ",
                       region.name);
 
body << "assert(odsAttrs && \"no attributes when constructing adapter\");\n"
                     attr.getStorageType());
 
     if (attr.hasDefaultValue() && attr.isOptional()) {
       // Use the default value if attribute is not set.
       // TODO: this is inefficient, we are recreating the attribute for every
       // call. This should be set instead.
       std::string defaultValue = std::string(
           tgfmt(attr.getConstBuilderTemplate(), &fctx, attr.getDefaultValue()));
-      body << "  if (!attr)\n    attr = " << defaultValue << ";\n";
+      body << "if (!attr)\n  attr = " << defaultValue << ";\n";
     }
     body << "return attr;\n";
   };
 
   {
     auto *m = adaptor.addMethod("::mlir::DictionaryAttr", "getAttributes");
     ERROR_IF_PRUNED(m, "Adaptor::getAttributes", op);
     m->body() << "  return odsAttrs;";