[mlir][tosa] Add dialect version.

[jpienaar]

This adds a singular number for the bytecode version. Considered adding spec related
version, but decided that against that as

1. I think one may want to capture the minimum spec to execute
   separately (and it may be function of the ops in the module);
2. Its unrelated to reading the bytecode or upgrade/downgrade. So
   linking these together felt like linking error domains.

[llvmbot]

@llvm/pr-subscribers-mlir-tosa

@llvm/pr-subscribers-mlir

Author: Jacques Pienaar (jpienaar)

Changes

This adds a singular number for the bytecode version. Considered adding spec related
version, but decided that against that as

1. I think one may want to capture the minimum spec to execute
   separately (and it may be function of the ops in the module);
2. Its unrelated to reading the bytecode or upgrade/downgrade. So
   linking these together felt like linking error domains.

---

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

2 Files Affected:

• (modified) mlir/include/mlir/Dialect/Tosa/IR/TosaOps.h (+7)
• (modified) mlir/lib/Dialect/Tosa/IR/TosaOps.cpp (+17-5)

diff --git a/mlir/include/mlir/Dialect/Tosa/IR/TosaOps.h b/mlir/include/mlir/Dialect/Tosa/IR/TosaOps.h
index a9bc3351f4cff05..062fb28f5ebe3cb 100644
--- a/mlir/include/mlir/Dialect/Tosa/IR/TosaOps.h
+++ b/mlir/include/mlir/Dialect/Tosa/IR/TosaOps.h
@@ -34,6 +34,13 @@ class PatternRewriter;
 
 namespace tosa {
 
+struct TosaDialectVersion : public mlir::DialectVersion {
+  TosaDialectVersion() = default;
+  TosaDialectVersion(uint32_t dialectVersion)
+      : dialectVersion(dialectVersion){};
+  uint32_t dialectVersion = 0;
+};
+
 ParseResult parseTypeOrAttr(OpAsmParser &parser, TypeAttr &typeAttr,
                             Attribute &attr);
 void printTypeOrAttr(OpAsmPrinter &p, Operation *op, TypeAttr type,
diff --git a/mlir/lib/Dialect/Tosa/IR/TosaOps.cpp b/mlir/lib/Dialect/Tosa/IR/TosaOps.cpp
index 729116da45e47dc..b4035cadce331a8 100644
--- a/mlir/lib/Dialect/Tosa/IR/TosaOps.cpp
+++ b/mlir/lib/Dialect/Tosa/IR/TosaOps.cpp
@@ -97,18 +97,30 @@ struct TosaDialectBytecodeInterface : public BytecodeDialectInterface {
   }
 
   void writeVersion(DialectBytecodeWriter &writer) const final {
-    // TODO: Populate.
+    // This is currently not being written currently to allow readers to update
+    // first.
+#if 0
+    // TODO: This could be refined to not just pick current version.
+    auto version = TosaDialectVersion();
+    writer.writeVarInt(version.dialectVersion);
+#endif
   }
 
   std::unique_ptr<DialectVersion>
   readVersion(DialectBytecodeReader &reader) const final {
-    // TODO: Populate
-    reader.emitError("Dialect does not support versioning");
-    return nullptr;
+    uint64_t dialectVersion;
+    if (failed(reader.readVarInt(dialectVersion)))
+      return nullptr;
+    auto version = std::make_unique<TosaDialectVersion>();
+    version->dialectVersion = dialectVersion;
+    return version;
   }
 
   LogicalResult upgradeFromVersion(Operation *topLevelOp,
-                                   const DialectVersion &version_) const final {
+                                   const DialectVersion &version) const final {
+    const auto &tosaVersion = static_cast<const TosaDialectVersion &>(version);
+    // No upgrades currently defined.
+    (void)tosaVersion;
     return success();
   }
 };

[eric-k256]

Looks good, I agree with your reasoning to stick to a singular number.

[joker-eph]

This lack some information to me:

• What is the purpose of this?
• What kind of compatibility is this supposed to provide?
• What is the versioning scheme?

[jpienaar]

This lack some information to me:

• What is the purpose of this?

To be able to flag incompatible bytecode files rather than have it fail later in mysterious ways. E.g., allows for a more strict failure.

• What kind of compatibility is this supposed to provide?

None today. Aspirational to use to avoid making cross repo changes in more atomic manner.

• What is the versioning scheme?

Version is just a single number but this doesn't mean this is versioned in any deep way vs being able to flag incompatible versions (some small update hooks such as for things like where i64 attribute is changed to i8 etc could be utilized, no promises yet on these existing for any extent of time). Main goal is really to reduce cross repo sync changes without initially any additional expectations.

Currently there aren't even any tests to flag if a version bump is needed. That would be nice, but no policy there yet.

[joker-eph]

To be able to flag incompatible bytecode files rather than have it fail later in mysterious ways. E.g., allows for a more strict failure.

That would require some sort of principles and policy around the changes that affect the serialization of this and the maintenance of this version number, I haven't seen a discussion about this: did I miss it?

Also, TOSA isn't a hermetic dialect: it includes other IR entities from other dialects, and changing these would impact this story as well.

[jpienaar]

To be able to flag incompatible bytecode files rather than have it fail later in mysterious ways. E.g., allows for a more strict failure.

That would require some sort of principles and policy around the changes that affect the serialization of this and the maintenance of this version number, I haven't seen a discussion about this: did I miss it?

Also, TOSA isn't a hermetic dialect: it includes other IR entities from other dialects, and changing these would impact this story as well.

I think you are assuming guarantees here that doesn't exist. This is best effort, allows for it/but doesn't make it happen nor enforcement or any such. Primarily around TOSA ops and which builtin dialect attributes are used. E.g., "bump this if you discover you also need a atomic change TF repo side" kind of level: this doesn't remove the need for the change TF repo side along with LLVM revision bump, but this does allow for folks using TF via pip and TOSA ingestion as well to have a binary interop that keeps working. No requirement being placed on community. While allowing folks that care about enabling such cases to have it working.

So this is a mechanism with policy TBD.

IR entities from other dialects

Correct changing builtin dialect serialization would affect this. I think we should consider enabling detecting that indeed. If the builtin dialect serialization changes, one could get some really random results :) Even considered tying those to the bytecode version itself given the impact a change could have. That is a good discussion to have but separate from this.

[joker-eph]

I think you are assuming guarantees here that doesn't exist.

I'm just going by you're writing :)
You wrote: "To be able to flag incompatible bytecode files rather than have it fail later in mysterious ways"
But my take is that you're not actually providing the ability to flag incompatible bytecode, instead I believe what you're providing is the ability to explicitly invalidate every existing serialized bytecode at some arbitrary points in time by bumping the version. In between these even where the version is bumped: you haven't changed anything and we can't flag incompatible bytecode.

So this is a mechanism with policy TBD.

Right, and I'd like a better story around how we'd handle this policy before landing this kind of code in-tree actually: this is a bit too much "YOLO" to me right now.