[Mlir-commits] [mlir] 8fe0691 - [mlir] Update when to check for non-null optional default attr

[Jacques Pienaar]

Author: Jacques Pienaar
Date: 2022-08-17T17:00:47-07:00
New Revision: 8fe06911611a70373a54f12023dd8c66147d2612

URL: https://github.com/llvm/llvm-project/commit/8fe06911611a70373a54f12023dd8c66147d2612
DIFF: https://github.com/llvm/llvm-project/commit/8fe06911611a70373a54f12023dd8c66147d2612.diff

LOG: [mlir] Update when to check for non-null optional default attr

OptionalAttr does the wrapping with Optional explicitly in ODS while
default valued optional attribute doesn't (and follows DefaultValuedAttr
in this behavior/meant as drop-in for updating old behavior), update
when to emit check for non-null to account for this.  Also add variant
for optional default valued string attribute to have same convenience as
default valued string attribute.

Added: 
    

Modified: 
    mlir/include/mlir/IR/OpBase.td
    mlir/test/mlir-tblgen/op-attribute.td
    mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

Removed: 
    


################################################################################
diff  --git a/mlir/include/mlir/IR/OpBase.td b/mlir/include/mlir/IR/OpBase.td
index 330d26cf9b740..cbacb90b08114 100644
--- a/mlir/include/mlir/IR/OpBase.td
+++ b/mlir/include/mlir/IR/OpBase.td
@@ -1014,6 +1014,8 @@ class OptionalAttr<Attr attr> : Attr<attr.predicate, attr.summary> {
 // quotes.
 class DefaultValuedStrAttr<Attr attr, string val>
     : DefaultValuedAttr<attr, "\"" # val # "\"">;
+class DefaultValuedOptionalStrAttr<Attr attr, string val>
+    : DefaultValuedOptionalAttr<attr, "\"" # val # "\"">;
 
 //===----------------------------------------------------------------------===//
 // Primitive attribute kinds

diff  --git a/mlir/test/mlir-tblgen/op-attribute.td b/mlir/test/mlir-tblgen/op-attribute.td
index 89a3bba32dac4..6c55fc45d38c9 100644
--- a/mlir/test/mlir-tblgen/op-attribute.td
+++ b/mlir/test/mlir-tblgen/op-attribute.td
@@ -34,7 +34,8 @@ def AOp : NS_Op<"a_op", []> {
   let arguments = (ins
       SomeAttr:$aAttr,
       DefaultValuedAttr<SomeAttr, "4.2">:$bAttr,
-      OptionalAttr<SomeAttr>:$cAttr
+      OptionalAttr<SomeAttr>:$cAttr,
+      DefaultValuedOptionalAttr<SomeAttr, "4.2">:$dAttr
   );
 }
 
@@ -45,7 +46,7 @@ def AOp : NS_Op<"a_op", []> {
 
 // DECL:      static ::llvm::ArrayRef<::llvm::StringRef> getAttributeNames()
 // DECL-NEXT:   static ::llvm::StringRef attrNames[] =
-// DECL-SAME:     {::llvm::StringRef("aAttr"), ::llvm::StringRef("bAttr"), ::llvm::StringRef("cAttr")};
+// DECL-SAME:     {::llvm::StringRef("aAttr"), ::llvm::StringRef("bAttr"), ::llvm::StringRef("cAttr"), ::llvm::StringRef("dAttr")};
 // DECL-NEXT:   return ::llvm::makeArrayRef(attrNames);
 
 // DECL:      ::mlir::StringAttr aAttrAttrName()
@@ -78,6 +79,7 @@ def AOp : NS_Op<"a_op", []> {
 // DEF-NEXT:     break;
 // DEF:      ::mlir::Attribute tblgen_bAttr;
 // DEF-NEXT: ::mlir::Attribute tblgen_cAttr;
+// DEF-NEXT: ::mlir::Attribute tblgen_dAttr;
 // DEF-NEXT: while (true) {
 // DEF-NEXT:   if (namedAttrIt == namedAttrRange.end())
 // DEF-NEXT:     break;
@@ -91,6 +93,8 @@ def AOp : NS_Op<"a_op", []> {
 // DEF-NEXT:   return emitError(loc, "'test.a_op' op ""attribute 'bAttr' failed to satisfy constraint: some attribute kind");
 // DEF:      if (tblgen_cAttr && !((some-condition)))
 // DEF-NEXT:   return emitError(loc, "'test.a_op' op ""attribute 'cAttr' failed to satisfy constraint: some attribute kind");
+// DEF:      if (tblgen_dAttr && !((some-condition)))
+// DEF-NEXT:   return emitError(loc, "'test.a_op' op ""attribute 'dAttr' failed to satisfy constraint: some attribute kind");
 
 // Test getter methods
 // ---
@@ -115,6 +119,14 @@ def AOp : NS_Op<"a_op", []> {
 // DEF-NEXT:   auto attr = cAttrAttr()
 // DEF-NEXT:   return attr ? ::llvm::Optional<some-return-type>(attr.some-convert-from-storage()) : (::llvm::None);
 
+// DEF:      some-attr-kind AOp::dAttrAttr()
+// DEF-NEXT:   ::mlir::impl::getAttrFromSortedRange((*this)->getAttrs().begin() + 1, (*this)->getAttrs().end() - 0, dAttrAttrName()).dyn_cast_or_null<some-attr-kind>()
+// DEF:      some-return-type AOp::dAttr() {
+// DEF-NEXT:   auto attr = dAttrAttr();
+// DEF-NEXT:   if (!attr)
+// DEF-NEXT:       return some-const-builder-call(::mlir::Builder((*this)->getContext()), 4.2).some-convert-from-storage();
+// DEF-NEXT:   return attr.some-convert-from-storage();
+
 // Test setter methods
 // ---
 
@@ -140,6 +152,14 @@ def AOp : NS_Op<"a_op", []> {
 // DEF:        if (cAttr) {
 // DEF-NEXT:     odsState.addAttribute(cAttrAttrName(odsState.name), cAttr);
 
+// DEF:        odsState.addAttribute(aAttrAttrName(odsState.name), some-const-builder-call(odsBuilder, aAttr));
+// DEF-NEXT:   odsState.addAttribute(bAttrAttrName(odsState.name), some-const-builder-call(odsBuilder, bAttr));
+// DEF-NEXT:   if (cAttr) {
+// DEF-NEXT:   odsState.addAttribute(cAttrAttrName(odsState.name), cAttr);
+// DEF-NEXT:   }
+// DEF-NOT:    if (dAttr)
+// DEF:        odsState.addAttribute(dAttrAttrName(odsState.name), some-const-builder-call(odsBuilder, dAttr));
+
 // DEF:      void AOp::build(
 // DEF:        some-return-type aAttr, some-return-type bAttr, /*optional*/some-attr-kind cAttr
 // DEF:        odsState.addAttribute(aAttrAttrName(odsState.name), some-const-builder-call(odsBuilder, aAttr));

diff  --git a/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp b/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
index 1efe6afccc9c4..c341591f807cc 100644
--- a/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
+++ b/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
@@ -2095,8 +2095,10 @@ void OpEmitter::genCodeForAddingArgAndRegionForBuilder(
     if (attr.isDerivedAttr() || inferredAttributes.contains(namedAttr.name))
       continue;
 
-    bool emitNotNullCheck =
-        attr.isOptional() || (attr.hasDefaultValue() && !isRawValueAttr);
+    // TODO(jpienaar): The wrapping of optional is 
diff erent for default or not,
+    // so don't unwrap for default ones that would fail below.
+    bool emitNotNullCheck = (attr.isOptional() && !attr.hasDefaultValue()) ||
+                            (attr.hasDefaultValue() && !isRawValueAttr);
     if (emitNotNullCheck)
       body << formatv("  if ({0}) ", namedAttr.name) << "{\n";