[Mlir-commits] [mlir] [mlir] Fix TableGen emission for ops without properties (PR #112851)

[lorenzo chelini]

https://github.com/chelini updated https://github.com/llvm/llvm-project/pull/112851

>From d5acba5752b9edf266e95040d25e5c6478edeb42 Mon Sep 17 00:00:00 2001
From: lorenzo chelini <lchelini at nvidia.com>
Date: Fri, 18 Oct 2024 10:16:24 +0200
Subject: [PATCH] [mlir] Fix emission of `prop-dict` when the operation does
 not have properties
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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);

```
---
 mlir/test/IR/properties.mlir                |  4 +++
 mlir/test/lib/Dialect/Test/TestOps.td       |  5 ++++
 mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp |  2 +-
 mlir/tools/mlir-tblgen/OpFormatGen.cpp      | 31 +++++++++++----------
 mlir/tools/mlir-tblgen/OpFormatGen.h        |  3 +-
 5 files changed, 29 insertions(+), 16 deletions(-)

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/OpDefinitionsGen.cpp b/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
index 71fa5011a476b4..dea6fb209863ce 100644
--- a/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
+++ b/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
@@ -1106,7 +1106,7 @@ OpEmitter::OpEmitter(const Operator &op,
   genFolderDecls();
   genTypeInterfaceMethods();
   genOpInterfaceMethods();
-  generateOpFormat(op, opClass);
+  generateOpFormat(op, opClass, emitHelper.hasProperties());
   genSideEffectInterfaceMethods();
 }
 void OpEmitter::emitDecl(
diff --git a/mlir/tools/mlir-tblgen/OpFormatGen.cpp b/mlir/tools/mlir-tblgen/OpFormatGen.cpp
index c99c71572bec23..3bf6f2f6d38176 100644
--- a/mlir/tools/mlir-tblgen/OpFormatGen.cpp
+++ b/mlir/tools/mlir-tblgen/OpFormatGen.cpp
@@ -339,10 +339,8 @@ struct OperationFormat {
     Optional
   };
 
-  OperationFormat(const Operator &op)
-      : useProperties(op.getDialect().usePropertiesForAttributes() &&
-                      !op.getAttributes().empty()),
-        opCppClassName(op.getCppClassName()) {
+  OperationFormat(const Operator &op, bool hasProperties)
+      : useProperties(hasProperties), opCppClassName(op.getCppClassName()) {
     operandTypes.resize(op.getNumOperands(), TypeResolution());
     resultTypes.resize(op.getNumResults(), TypeResolution());
 
@@ -397,7 +395,7 @@ struct OperationFormat {
   /// A flag indicating if this operation has the SingleBlock trait.
   bool hasSingleBlockTrait;
 
-  /// Indicate whether attribute are stored in properties.
+  /// Indicate whether we need to use properties for the current operator.
   bool useProperties;
 
   /// Indicate whether prop-dict is used in the format
@@ -1275,8 +1273,8 @@ 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 +1619,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 +2047,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.
@@ -3771,7 +3773,8 @@ LogicalResult OpFormatParser::verifyOptionalGroupElement(SMLoc loc,
 // Interface
 //===----------------------------------------------------------------------===//
 
-void mlir::tblgen::generateOpFormat(const Operator &constOp, OpClass &opClass) {
+void mlir::tblgen::generateOpFormat(const Operator &constOp, OpClass &opClass,
+                                    bool hasProperties) {
   // TODO: Operator doesn't expose all necessary functionality via
   // the const interface.
   Operator &op = const_cast<Operator &>(constOp);
@@ -3782,7 +3785,7 @@ void mlir::tblgen::generateOpFormat(const Operator &constOp, OpClass &opClass) {
   llvm::SourceMgr mgr;
   mgr.AddNewSourceBuffer(
       llvm::MemoryBuffer::getMemBuffer(op.getAssemblyFormat()), SMLoc());
-  OperationFormat format(op);
+  OperationFormat format(op, hasProperties);
   OpFormatParser parser(mgr, format, op);
   FailureOr<std::vector<FormatElement *>> elements = parser.parse();
   if (failed(elements)) {
diff --git a/mlir/tools/mlir-tblgen/OpFormatGen.h b/mlir/tools/mlir-tblgen/OpFormatGen.h
index 88dbc99d9f78ef..5e43f38498664b 100644
--- a/mlir/tools/mlir-tblgen/OpFormatGen.h
+++ b/mlir/tools/mlir-tblgen/OpFormatGen.h
@@ -20,7 +20,8 @@ class OpClass;
 class Operator;
 
 // Generate the assembly format for the given operator.
-void generateOpFormat(const Operator &constOp, OpClass &opClass);
+void generateOpFormat(const Operator &constOp, OpClass &opClass,
+                      bool hasProperties);
 
 } // namespace tblgen
 } // namespace mlir