[mlir][bytecode] Fix D155919 and enable backward-compatibility and back-deployment between version 5 and version 6 of the bytecode encoding

[mfrancio]

Before D155919, when a dialect was leveraging properties to store attributes with usePropertiesForAttributes, the operand segment sizes attribute was emitted in the property section in sorted order together with all the other attributes of an op. After D155919, version 5 of the bytecode was emitting and parsing operand segment sizes after all the properties of an op, breaking backward compatibility and back deployment. This patch fixes the emission ordering and allows to parse bytecode files emitted before (D155919) with version 5 of MLIR bytecode. The patch also enables to emit correctly version 5 of MLIR bytecode.

[llvmbot]

@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-mlir-core

Author: Matteo Franciolini (mfrancio)

Changes

…ck-deployment between version 5 and version 6 of the bytecode encoding.

Before D155919, when a dialect was leveraging properties to store attributes with usePropertiesForAttributes, the operand segment sizes attribute was emitted in the property section in sorted order together with all the other attributes of an op. After D155919, version 5 of the bytecode was emitting and parsing operand segment sizes after all the properties of an op, breaking backward compatibility and back deployment. This patch fixes the emission ordering and allows to parse bytecode files emitted before (D155919) with version 5 of MLIR bytecode. The patch also enables to emit correctly version 5 of MLIR bytecode.

---

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

1 Files Affected:

• (modified) mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp (+73-23)

diff --git a/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp b/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
index 7029c0eac15c392..6deb111d228f0ae 100644
--- a/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
+++ b/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
@@ -156,28 +156,38 @@ static const char *const valueRangeReturnCode = R"(
 )";
 
 /// Read operand/result segment_size from bytecode.
-static const char *const readBytecodeSegmentSize = R"(
-if ($_reader.getBytecodeVersion() < /*kNativePropertiesODSSegmentSize=*/6) {
-  ::mlir::DenseI32ArrayAttr attr;
-  if (::mlir::failed($_reader.readAttribute(attr))) return ::mlir::failure();
-  if (attr.size() > static_cast<int64_t>(sizeof($_storage) / sizeof(int32_t))) {
-    $_reader.emitError("size mismatch for operand/result_segment_size");
-    return ::mlir::failure();
-  }
-  ::llvm::copy(::llvm::ArrayRef<int32_t>(attr), $_storage.begin());
-} else {
+static const char *const readBytecodeSegmentSizeNative = R"(
+if ($_reader.getBytecodeVersion() >= /*kNativePropertiesODSSegmentSize=*/6)
   return $_reader.readSparseArray(::llvm::MutableArrayRef($_storage));
-}
+)";
+
+static const char *const readBytecodeSegmentSizeLegacy = R"(
+  if ($_reader.getBytecodeVersion() < /*kNativePropertiesODSSegmentSize=*/6) {
+    auto &$_storage = prop.$_propName;
+    ::mlir::DenseI32ArrayAttr attr;
+    if (::mlir::failed($_reader.readAttribute(attr))) return ::mlir::failure();
+    if (attr.size() > static_cast<int64_t>(sizeof($_storage) / sizeof(int32_t))) {
+      $_reader.emitError("size mismatch for operand/result_segment_size");
+      return ::mlir::failure();
+    }
+    ::llvm::copy(::llvm::ArrayRef<int32_t>(attr), $_storage.begin());
+  }
 )";
 
 /// Write operand/result segment_size to bytecode.
-static const char *const writeBytecodeSegmentSize = R"(
-if ($_writer.getBytecodeVersion() < /*kNativePropertiesODSSegmentSize=*/6)
-  $_writer.writeAttribute(::mlir::DenseI32ArrayAttr::get(getContext(), $_storage));
-else
+static const char *const writeBytecodeSegmentSizeNative = R"(
+if ($_writer.getBytecodeVersion() >= /*kNativePropertiesODSSegmentSize=*/6)
   $_writer.writeSparseArray(::llvm::ArrayRef($_storage));
 )";
 
+/// Write operand/result segment_size to bytecode.
+static const char *const writeBytecodeSegmentSizeLegacy = R"(
+if ($_writer.getBytecodeVersion() < /*kNativePropertiesODSSegmentSize=*/6) {
+  auto &$_storage = prop.$_propName;
+  $_writer.writeAttribute(::mlir::DenseI32ArrayAttr::get($_ctxt, $_storage));
+}
+)";
+
 /// A header for indicating code sections.
 ///
 /// {0}: Some text, or a class name.
@@ -389,6 +399,14 @@ class OpOrAdaptorHelper {
     return resultSegmentsSize;
   }
 
+  unsigned long getOperandSegmentSizesLegacyIndex() {
+    return operandSegmentSizesLegacyIndex;
+  }
+
+  unsigned long getResultSegmentSizesLegacyIndex() {
+    return resultSegmentSizesLegacyIndex;
+  }
+
 private:
   // Compute the attribute metadata.
   void computeAttrMetadata();
@@ -406,6 +424,8 @@ class OpOrAdaptorHelper {
   std::string operandSegmentsSizeStorage;
   std::optional<NamedProperty> resultSegmentsSize;
   std::string resultSegmentsSizeStorage;
+  unsigned long operandSegmentSizesLegacyIndex = 0;
+  unsigned long resultSegmentSizesLegacyIndex = 0;
 
   // The number of required attributes.
   unsigned numRequired;
@@ -435,8 +455,8 @@ void OpOrAdaptorHelper::computeAttrMetadata() {
         "::mlir::DenseI32ArrayAttr::get($_ctxt, $_storage)",
         /*convertFromAttributeCall=*/
         "return convertFromAttribute($_storage, $_attr, $_diag);",
-        /*readFromMlirBytecodeCall=*/readBytecodeSegmentSize,
-        /*writeToMlirBytecodeCall=*/writeBytecodeSegmentSize,
+        /*readFromMlirBytecodeCall=*/readBytecodeSegmentSizeNative,
+        /*writeToMlirBytecodeCall=*/writeBytecodeSegmentSizeNative,
         /*hashPropertyCall=*/
         "::llvm::hash_combine_range(std::begin($_storage), "
         "std::end($_storage));",
@@ -478,6 +498,21 @@ void OpOrAdaptorHelper::computeAttrMetadata() {
                return lhs.attrName < rhs.attrName;
              });
 
+  // Store the position of the legacy operand_segment_sizes /
+  // result_segment_sizes so we can emit a backward compatible property readers
+  // and writers.
+  StringRef oldName = StringLiteral("operand_segment_sizes");
+  operandSegmentSizesLegacyIndex =
+      llvm::count_if(sortedAttrMetadata, [&](auto metaData) {
+        return metaData.attrName < oldName;
+      });
+
+  oldName = StringLiteral("result_segment_sizes");
+  resultSegmentSizesLegacyIndex =
+      llvm::count_if(sortedAttrMetadata, [&](auto metaData) {
+        return metaData.attrName < oldName;
+      });
+
   // Compute the subrange bounds for each attribute.
   numRequired = 0;
   for (AttributeMetadata &attr : sortedAttrMetadata) {
@@ -1580,15 +1615,30 @@ void OpEmitter::genPropertiesSupportForBytecode(
   readPropertiesMethod
       << "  auto &prop = state.getOrAddProperties<Properties>(); (void)prop;";
   writePropertiesMethod << "  auto &prop = getProperties(); (void)prop;\n";
-  for (const auto &attrOrProp : attrOrProperties) {
+  for (const auto &item : llvm::enumerate(attrOrProperties)) {
+    auto &attrOrProp = item.value();
+    FmtContext fctx;
+    fctx.addSubst("_reader", "reader")
+        .addSubst("_writer", "writer")
+        .addSubst("_storage", propertyStorage)
+        .addSubst("_ctxt", "this->getContext()");
+    if (emitHelper.getOperandSegmentsSize().has_value() &&
+        item.index() == emitHelper.getOperandSegmentSizesLegacyIndex()) {
+      FmtContext fmtCtxt(fctx);
+      fmtCtxt.addSubst("_propName", operandSegmentAttrName);
+      readPropertiesMethod << tgfmt(readBytecodeSegmentSizeLegacy, &fmtCtxt);
+      writePropertiesMethod << tgfmt(writeBytecodeSegmentSizeLegacy, &fmtCtxt);
+    }
+    if (emitHelper.getResultSegmentsSize().has_value() &&
+        item.index() == emitHelper.getResultSegmentSizesLegacyIndex()) {
+      FmtContext fmtCtxt(fctx);
+      fmtCtxt.addSubst("_propName", operandSegmentAttrName);
+      readPropertiesMethod << tgfmt(readBytecodeSegmentSizeLegacy, &fmtCtxt);
+      writePropertiesMethod << tgfmt(writeBytecodeSegmentSizeLegacy, &fmtCtxt);
+    }
     if (const auto *namedProperty =
             attrOrProp.dyn_cast<const NamedProperty *>()) {
       StringRef name = namedProperty->name;
-      FmtContext fctx;
-      fctx.addSubst("_reader", "reader")
-          .addSubst("_writer", "writer")
-          .addSubst("_storage", propertyStorage)
-          .addSubst("_ctxt", "this->getContext()");
       readPropertiesMethod << formatv(
           R"(
   {{

[joker-eph]

Add a short doc here please

[joker-eph]

Prefer: int64_t or int

[joker-eph]

Add a comment for the block around the legacy stuff

[joker-eph]

Please edit the PR and fix the title/description, GitHub wrapped it.