[mlir] Fix the emission of prop-dict when operations have no properties

[chelini]

When an operation has no properties, no property struct is emitted. To avoid a
compilation error, we should also skip emitting setPropertiesFromParsedAttr,
parseProperties and printProperties in such cases.

Compilation error:

    error: ‘Properties’ has not been declared
      static ::llvm::LogicalResult setPropertiesFromParsedAttr(Properties &prop, ::mlir::Attribute attr, ::llvm::function_ref<::mlir::InFlightDiagnostic()> emitError);
    

[llvmbot]

@llvm/pr-subscribers-mlir-core

@llvm/pr-subscribers-mlir

Author: lorenzo chelini (chelini)

Changes

The current condition to emit setPropertiesFromParsedAttr is not strict enough since we emit the method even when we do not have properties at all. Similarly we should not emit parseProperties and printProperties if we don't have properties.

---

Full diff: https://github.com/llvm/llvm-project/pull/112851.diff

3 Files Affected:

• (modified) mlir/test/IR/properties.mlir (+4)
• (modified) mlir/test/lib/Dialect/Test/TestOps.td (+5)
• (modified) mlir/tools/mlir-tblgen/OpFormatGen.cpp (+12-7)

diff --git a/mlir/test/IR/properties.mlir b/mlir/test/IR/properties.mlir
index 418b81dcbb034f..9a1c49cb7dabf3 100644
--- a/mlir/test/IR/properties.mlir
+++ b/mlir/test/IR/properties.mlir
@@ -19,6 +19,10 @@ test.with_nice_properties "foo bar" is -3
 // GENERIC-SAME:  <{prop = "content for properties"}> : () -> ()
 test.with_wrapped_properties <{prop = "content for properties"}>
 
+// CHECK: test.empty_properties
+// GENERIC: "test.empty_properties"()
+test.empty_properties
+
 // CHECK: test.using_property_in_custom
 // CHECK-SAME: [1, 4, 20]{{$}}
 // GENERIC: "test.using_property_in_custom"()
diff --git a/mlir/test/lib/Dialect/Test/TestOps.td b/mlir/test/lib/Dialect/Test/TestOps.td
index 9e19966414d1d7..bc6c6cf213ea4b 100644
--- a/mlir/test/lib/Dialect/Test/TestOps.td
+++ b/mlir/test/lib/Dialect/Test/TestOps.td
@@ -3006,6 +3006,11 @@ def TestOpWithWrappedProperties : TEST_Op<"with_wrapped_properties"> {
   );
 }
 
+def TestOpWithEmptyProperties : TEST_Op<"empty_properties"> {
+  let assemblyFormat = "prop-dict attr-dict";
+  let arguments = (ins);
+}
+
 def TestOpUsingPropertyInCustom : TEST_Op<"using_property_in_custom"> {
   let assemblyFormat = "custom<UsingPropertyInCustom>($prop) attr-dict";
   let arguments = (ins IntArrayProperty<"int64_t">:$prop);
diff --git a/mlir/tools/mlir-tblgen/OpFormatGen.cpp b/mlir/tools/mlir-tblgen/OpFormatGen.cpp
index c99c71572bec23..4703de67025ded 100644
--- a/mlir/tools/mlir-tblgen/OpFormatGen.cpp
+++ b/mlir/tools/mlir-tblgen/OpFormatGen.cpp
@@ -1275,8 +1275,9 @@ static void genAttrParser(AttributeVariable *attr, MethodBody &body,
 // 'prop-dict' dictionary attr.
 static void genParsedAttrPropertiesSetter(OperationFormat &fmt, Operator &op,
                                           OpClass &opClass) {
-  // Not required unless 'prop-dict' is present.
-  if (!fmt.hasPropDict)
+  // Not required unless 'prop-dict' is present or we
+  // are not using properties.
+  if (!fmt.hasPropDict || !fmt.useProperties)
     return;
 
   SmallVector<MethodParameter> paramList;
@@ -1621,8 +1622,10 @@ void OperationFormat::genElementParser(FormatElement *element, MethodBody &body,
     body.unindent() << "}\n";
     body.unindent();
   } else if (isa<PropDictDirective>(element)) {
-    body << "  if (parseProperties(parser, result))\n"
-         << "    return ::mlir::failure();\n";
+    if (useProperties) {
+      body << "  if (parseProperties(parser, result))\n"
+           << "    return ::mlir::failure();\n";
+    }
   } else if (auto *customDir = dyn_cast<CustomDirective>(element)) {
     genCustomDirectiveParser(customDir, body, useProperties, opCppClassName);
   } else if (isa<OperandsDirective>(element)) {
@@ -2047,9 +2050,11 @@ static void genPropDictPrinter(OperationFormat &fmt, Operator &op,
     }
   }
 
-  body << "  _odsPrinter << \" \";\n"
-       << "  printProperties(this->getContext(), _odsPrinter, "
-          "getProperties(), elidedProps);\n";
+  if (fmt.useProperties) {
+    body << "  _odsPrinter << \" \";\n"
+         << "  printProperties(this->getContext(), _odsPrinter, "
+            "getProperties(), elidedProps);\n";
+  }
 }
 
 /// Generate the printer for the 'attr-dict' directive.

[zero9178]

Curious about the motivation here: Is there a problem with always emitting these methods that this PR solves?

[chelini]

Curious about the motivation here: Is there a problem with always emitting these methods that this PR solves?

yes, if we did not emit any property struct for the current operation we should not inject these method, otherwise you end up with a compilation error.

error: ‘Properties’ has not been declared
46380 |   static ::llvm::LogicalResult setPropertiesFromParsedAttr(Properties &prop, ::mlir::Attribute attr, ::llvm::function_ref<::mlir::InFlightDiagnostic()> emitError);

[River707]

For a second I wasn't sure what was broken from the description, but it seems like this PR fixes emission of prop-dict when the operation doesn't have properties?

[joker-eph]

Thanks for the fix!

Can you please edit the description to make it more clear what is the problem fixed here?

[chelini]

For a second I wasn't sure what was broken from the description, but it seems like this PR fixes emission of prop-dict when the operation doesn't have properties?

yes, correct. Let me improve the commit msg.

[chelini]

Thanks for the fix!

Can you please edit the description to make it more clear what is the problem fixed here?

Thanks, updated. Hopefully it is more clear now!