[Mlir-commits] [mlir] [mlir][bytecode] Reject lossy legacy property serialization (PR #233216)

[llvmlistbot at llvm.org]

llvmorg-github-actions[bot] wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-mlir-core

Author: Mehdi Amini (joker-eph)

<details>
<summary>Changes</summary>

Bytecode versions before 5 serialize operation properties through the legacy attribute dictionary. Native properties omitted from that dictionary silently reset to their defaults when read back. For example, test.with_versioned_properties 1 | 2 becomes 0 | 0 with version 0.

Reconstruct the properties from the legacy dictionary and compare them with the original storage before emitting the operation. Reject writes that would lose property values while retaining support for default native properties and attribute-backed fields.

Cover versions 0 through 6, including the native encoding boundary.

Assisted-by: Codex

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


2 Files Affected:

- (modified) mlir/lib/Bytecode/Writer/BytecodeWriter.cpp (+14) 
- (added) mlir/test/Bytecode/legacy-properties.mlir (+24) 


``````````diff
diff --git a/mlir/lib/Bytecode/Writer/BytecodeWriter.cpp b/mlir/lib/Bytecode/Writer/BytecodeWriter.cpp
index 89aa731c5cca05..ddec6ca793e1f6 100644
--- a/mlir/lib/Bytecode/Writer/BytecodeWriter.cpp
+++ b/mlir/lib/Bytecode/Writer/BytecodeWriter.cpp
@@ -1028,6 +1028,20 @@ LogicalResult BytecodeWriter::writeOp(EncodingEmitter &emitter, Operation *op) {
     });
     allAttrs.append(op->getDiscardableAttrDictionary().getValue());
     attrs = allAttrs.getDictionary(op->getContext());
+
+    // Legacy bytecode can only reconstruct properties from these attributes.
+    // Check that native properties would not silently revert to their defaults.
+    OperationState legacyState(op->getLoc(), op->getName());
+    legacyState.addAttributes(attrs.getValue());
+    Operation *legacyOp = Operation::create(legacyState);
+    bool preservesProperties = op->getName().compareOpProperties(
+        op->getPropertiesStorage(), legacyOp->getPropertiesStorage());
+    legacyOp->destroy();
+    if (!preservesProperties)
+      return op->emitOpError()
+             << "cannot emit bytecode before version "
+             << static_cast<int64_t>(bytecode::kNativePropertiesEncoding)
+             << " without losing operation properties";
   }
   if (!attrs.empty()) {
     opEncodingMask |= bytecode::OpEncodingMask::kHasAttrs;
diff --git a/mlir/test/Bytecode/legacy-properties.mlir b/mlir/test/Bytecode/legacy-properties.mlir
new file mode 100644
index 00000000000000..42382eca2be31a
--- /dev/null
+++ b/mlir/test/Bytecode/legacy-properties.mlir
@@ -0,0 +1,24 @@
+// RUN: mlir-opt %s -split-input-file -emit-bytecode -emit-bytecode-version=0 -verify-diagnostics
+// RUN: mlir-opt %s -split-input-file -emit-bytecode -emit-bytecode-version=1 -verify-diagnostics
+// RUN: mlir-opt %s -split-input-file -emit-bytecode -emit-bytecode-version=2 -verify-diagnostics
+// RUN: mlir-opt %s -split-input-file -emit-bytecode -emit-bytecode-version=3 -verify-diagnostics
+// RUN: mlir-opt %s -split-input-file -emit-bytecode -emit-bytecode-version=4 -verify-diagnostics
+// RUN: mlir-opt %s -emit-bytecode -emit-bytecode-version=5 | mlir-opt | FileCheck %s
+// RUN: mlir-opt %s -emit-bytecode -emit-bytecode-version=6 | mlir-opt | FileCheck %s
+
+// Native properties cannot be merged into the legacy attribute dictionary.
+// CHECK: test.with_versioned_properties 1 | 2
+// expected-error at +1 {{cannot emit bytecode before version 5 without losing operation properties}}
+test.with_versioned_properties 1 | 2
+
+// -----
+
+// Default native properties can still round-trip through a legacy format.
+// CHECK: test.with_versioned_properties 0 | 0
+test.with_versioned_properties 0 | 0
+
+// -----
+
+// Inherent attributes can be reconstructed from the legacy dictionary.
+// CHECK: "test.versionedA"() <{dims = 123 : i64, modifier = true}>
+"test.versionedA"() <{dims = 123 : i64, modifier = true}> : () -> ()

``````````

</details>


https://github.com/llvm/llvm-project/pull/233216