[Mlir-commits] [mlir] [MLIR] Consider properties when populating default attrs (PR #78525)

[Billy Zhu]

https://github.com/zyx-billy created https://github.com/llvm/llvm-project/pull/78525

Currently DefaultValuedAttrs and Properties don't play well with each other.
- When an Op has any DefaultValuedAttrs, ODS generates a `populateDefaultAttrs(NamedAttrList &&)` method to add attributes that are missing from the existing list.
- When an Op has any properties, all of its attributes are stored as properties.

Now when creating an Operation abstractly using `Operation::create(const OperationState &state)` (for example, when reading bytecode), `state.properties` will be filled with the existing attributes, while `state.attributes` will be empty. This causes `populateDefaultAttrs` to always fill in all default attributes, even if they exist inside properties already. These default attributes then overwrite the pre-existing attributes inside properties when they're set onto the op at the end of Operation::create.

This PR switches the order so that properties are also considered when populating default attrs.

>From 491c0942b3da46b5ed6c00742b559917ec131829 Mon Sep 17 00:00:00 2001
From: Billy Zhu <billyzhu at modular.com>
Date: Wed, 17 Jan 2024 16:38:04 -0800
Subject: [PATCH] consider properties when populating default attrs

---
 mlir/lib/IR/Operation.cpp | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/mlir/lib/IR/Operation.cpp b/mlir/lib/IR/Operation.cpp
index 311f5bb5ef77c0e..7646fdae6fb503f 100644
--- a/mlir/lib/IR/Operation.cpp
+++ b/mlir/lib/IR/Operation.cpp
@@ -69,9 +69,6 @@ Operation *Operation::create(Location location, OperationName name,
                              NamedAttrList &&attributes,
                              OpaqueProperties properties, BlockRange successors,
                              unsigned numRegions) {
-  // Populate default attributes.
-  name.populateDefaultAttrs(attributes);
-
   return create(location, name, resultTypes, operands,
                 attributes.getDictionary(location.getContext()), properties,
                 successors, numRegions);
@@ -146,8 +143,13 @@ Operation *Operation::create(Location location, OperationName name,
   for (unsigned i = 0; i != numSuccessors; ++i)
     new (&blockOperands[i]) BlockOperand(op, successors[i]);
 
-  // This must be done after properties are initalized.
-  op->setAttrs(attributes);
+  // Populate default attributes.
+  // This must be done after any properties are initalized.
+  NamedAttrList attrList(attributes);
+  if (opPropertiesAllocSize)
+    attrList.append(op->getAttrDictionary());
+  name.populateDefaultAttrs(attrList);
+  op->setAttrs(attrList);
 
   return op;
 }