Use symbolic name for previous MLIR Bytecode versions

[burmako]

mlir/include/mlir/Bytecode/Encoding.h

46

Very nice! I think this will convention will considerably simplify maintenance and minimize the effort needed to rebase bytecode-related patches on top of other bytecode-related patches.

mlir/lib/Bytecode/Reader/BytecodeReader.cpp

1701–1702

This is one my first upstream reviews, so I'm not sure what the convention is, but you may want to reformat lines like this one to fit into 80 characters.

mlir/lib/Bytecode/Writer/BytecodeWriter.cpp

735

How about config.bytecodeVersion >= bytecode::kElideUnknownBlockArgLocation?

[mehdi_amini]

clang-format

[mehdi_amini]

Address comment: fix one comparison

[mehdi_amini]

mlir/lib/Bytecode/Reader/BytecodeReader.cpp

1701–1702

Uh, somehow arc lint does not work on my laptop :(
(usually clang-format runs automatically when I create a revision)

[jpienaar]

Nice, serves as a version log too. Could we do something so that doxygen generates a nice table for it?

[mehdi_amini]

Doxygen will print a table:

mlir/include/mlir/Bytecode/Encoding.h

 #include <type_traits>
 
 namespace mlir {
 namespace bytecode {
 //===----------------------------------------------------------------------===//
 // General constants
 //===----------------------------------------------------------------------===//
 
-enum {
+enum BytecodeVersion {
   /// The minimum supported version of the bytecode.
   kMinSupportedVersion = 0,
 
+  /// Dialects versioning was added in version 1.
+  kDialectVersioning = 1,
+
+  /// Support for lazy-loading of isolated region was added in version 2.
+  kLazyLoading = 2,
+
+  /// Use-list ordering started to be encoded in version 3.
+  kUseListOrdering = 3,
+
+  /// Avoid recording unknown locations on block arguments (compression) started
+  /// in version 4.
+  kElideUnknownBlockArgLocation = 4,
+
+  /// Support for encoding properties natively in bytecode instead of merged
+  /// with the discardable attributes.
+  kNativePropertiesEncoding = 5,
+
   /// The current bytecode version.
   kVersion = 5,
 
   /// An arbitrary value used to fill alignment padding.
   kAlignmentByte = 0xCB,
 };
 
 //===----------------------------------------------------------------------===//

mlir/lib/Bytecode/Reader/BytecodeReader.cpp

static bool isSectionOptional(bytecode::Section::ID sectionID, int version) {
   case bytecode::Section::kAttrTypeOffset:
   case bytecode::Section::kIR:
     return false;
   case bytecode::Section::kResource:
   case bytecode::Section::kResourceOffset:
   case bytecode::Section::kDialectVersions:
     return true;
   case bytecode::Section::kProperties:
-    return version < 5;
+    return version < bytecode::kNativePropertiesEncoding;
   default:
     llvm_unreachable("unknown section ID");
   }
 }
 
 //===----------------------------------------------------------------------===//
 // EncodingReader
 //===----------------------------------------------------------------------===//
struct BytecodeOperationName {
 
   /// The dialect that owns this operation name.
   BytecodeDialect *dialect;
 
   /// The name of the operation, without the dialect prefix.
   StringRef name;
 
   /// Whether this operation was registered when the bytecode was produced.
-  /// This flag is populated when bytecode version >=5.
+  /// This flag is populated when bytecode version >=kNativePropertiesEncoding.
   std::optional<bool> wasRegistered;
 };
 } // namespace
 
 /// Parse a single dialect group encoded in the byte stream.
 static LogicalResult parseDialectGrouping(
     EncodingReader &reader, MutableArrayRef<BytecodeDialect> dialects,
     function_ref<LogicalResult(BytecodeDialect *)> entryCallback) {
return reader.emitError("bytecode version ", version,
                             currentVersion, ", and upgrade is not supported");
   }
   if (version > currentVersion) {
     return reader.emitError("bytecode version ", version,
                             " is newer than the current version ",
                             currentVersion);
   }
   // Override any request to lazy-load if the bytecode version is too old.
-  if (version < 2)
+  if (version < bytecode::kLazyLoading)
     lazyLoading = false;
   return success();
 }
 
 //===----------------------------------------------------------------------===//
 // Dialect Section
 
 LogicalResult BytecodeDialect::load(DialectReader &reader, MLIRContext *ctx) {
BytecodeReader::Impl::parseDialectSection(ArrayRef<uint8_t> sectionData) {
 
   // Parse the number of dialects in the section.
   uint64_t numDialects;
   if (failed(sectionReader.parseVarInt(numDialects)))
     return failure();
   dialects.resize(numDialects);
 
   // Parse each of the dialects.
   for (uint64_t i = 0; i < numDialects; ++i) {
-    /// Before version 1, there wasn't any versioning available for dialects,
-    /// and the entryIdx represent the string itself.
-    if (version == 0) {
+    /// Before version kDialectVersioning, there wasn't any versioning available
+    /// for dialects, and the entryIdx represent the string itself.
+    if (version < bytecode::kDialectVersioning) {
       if (failed(stringReader.parseString(sectionReader, dialects[i].name)))
         return failure();
       continue;
     }
     // Parse ID representing dialect and version.
     uint64_t dialectNameIdx;
     bool versionAvailable;
     if (failed(sectionReader.parseVarIntWithFlag(dialectNameIdx,
if (versionAvailable) {
       }
     }
   }
 
   // Parse the operation names, which are grouped by dialect.
   auto parseOpName = [&](BytecodeDialect *dialect) {
     StringRef opName;
     std::optional<bool> wasRegistered;
-    // Prior to version 5, the information about wheter an op was registered or
-    // not wasn't encoded.
-    if (version < 5) {
+    // Prior to version kNativePropertiesEncoding, the information about wheter
+    // an op was registered or not wasn't encoded.
+    if (version < bytecode::kNativePropertiesEncoding) {
       if (failed(stringReader.parseString(sectionReader, opName)))
         return failure();
     } else {
       bool wasRegisteredFlag;
       if (failed(stringReader.parseStringWithFlag(sectionReader, opName,
                                                   wasRegisteredFlag)))
         return failure();
       wasRegistered = wasRegisteredFlag;
     }
     opNames.emplace_back(dialect, opName, wasRegistered);
     return success();
   };
-  // Avoid re-allocation in bytecode version > 3 where the number of ops are
-  // known.
-  if (version > 3) {
+  // Avoid re-allocation in bytecode version >=kElideUnknownBlockArgLocation
+  // where the number of ops are known.
+  if (version >= bytecode::kElideUnknownBlockArgLocation) {
     uint64_t numOps;
     if (failed(sectionReader.parseVarInt(numOps)))
       return failure();
     opNames.reserve(numOps);
   }
   while (!sectionReader.empty())
     if (failed(parseDialectGrouping(sectionReader, dialects, parseOpName)))
       return failure();
do {
         // If the op has regions, add it to the stack for processing and return:
         // we stop the processing of the current region and resume it after the
         // inner one is completed. Unless LazyLoading is activated in which case
         // nested region parsing is delayed.
         if ((*op)->getNumRegions()) {
           RegionReadState childState(*op, &reader, isIsolatedFromAbove);
 
           // Isolated regions are encoded as a section in version 2 and above.
-          if (version >= 2 && isIsolatedFromAbove) {
+          if (version >= bytecode::kLazyLoading && isIsolatedFromAbove) {
             bytecode::Section::ID sectionID;
             ArrayRef<uint8_t> sectionData;
             if (failed(reader.parseSection(sectionID, sectionData)))
               return failure();
             if (sectionID != bytecode::Section::kIR)
               return emitError(fileLoc, "expected IR section for region");
             childState.owningReader =
                 std::make_unique<EncodingReader>(sectionData, fileLoc);
for (int i = 0, e = numSuccs; i < e; ++i) {
                             "successor")))
         return failure();
     }
   }
 
   /// Parse the use-list orders for the results of the operation. Use-list
   /// orders are available since version 3 of the bytecode.
   std::optional<UseListMapT> resultIdxToUseListMap = std::nullopt;
-  if (version > 2 && (opMask & bytecode::OpEncodingMask::kHasUseListOrders)) {
+  if (version >= bytecode::kUseListOrdering &&
+      (opMask & bytecode::OpEncodingMask::kHasUseListOrders)) {
     size_t numResults = opState.types.size();
     auto parseResult = parseUseListOrderForRange(reader, numResults);
     if (failed(parseResult))
       return failure();
     resultIdxToUseListMap = std::move(*parseResult);
   }
 
   /// Parse the regions of the operation.
BytecodeReader::Impl::parseBlockHeader(EncodingReader &reader,
   if (failed(reader.parseVarIntWithFlag(readState.numOpsRemaining, hasArgs)))
     return failure();
 
   // Parse the arguments of the block.
   if (hasArgs && failed(parseBlockArguments(reader, &*readState.curBlock)))
     return failure();
 
   // Uselist orders are available since version 3 of the bytecode.
-  if (version < 3)
+  if (version < bytecode::kUseListOrdering)
     return success();
 
   uint8_t hasUseListOrders = 0;
   if (hasArgs && failed(reader.parseByte(hasUseListOrders)))
     return failure();
 
   if (!hasUseListOrders)
     return success();
LogicalResult BytecodeReader::Impl::parseBlockArguments(EncodingReader &reader,
   SmallVector<Location> argLocs;
   argTypes.reserve(numArgs);
   argLocs.reserve(numArgs);
 
   Location unknownLoc = UnknownLoc::get(config.getContext());
   while (numArgs--) {
     Type argType;
     LocationAttr argLoc = unknownLoc;
-    if (version > 3) {
+    if (version >= bytecode::kElideUnknownBlockArgLocation) {
       // Parse the type with hasLoc flag to determine if it has type.
       uint64_t typeIdx;
       bool hasLoc;
       if (failed(reader.parseVarIntWithFlag(typeIdx, hasLoc)) ||
           !(argType = attrTypeReader.resolveType(typeIdx)))
         return failure();
       if (hasLoc && failed(parseAttribute(reader, argLoc)))
         return failure();

mlir/lib/Bytecode/Writer/BytecodeWriter.cpp

LogicalResult BytecodeWriter::write(Operation *rootOp, raw_ostream &os) {
 
   // Emit the resources section.
   writeResourceSection(rootOp, emitter);
 
   // Emit the string section.
   writeStringSection(emitter);
 
   // Emit the properties section.
-  if (config.bytecodeVersion >= 5)
+  if (config.bytecodeVersion >= bytecode::kNativePropertiesEncoding)
     writePropertiesSection(emitter);
   else if (!propertiesSection.empty())
     return rootOp->emitError(
         "unexpected properties emitted incompatible with bytecode <5");
 
   // Write the generated bytecode to the provided output stream.
   emitter.writeTo(os);
 
void BytecodeWriter::writeDialectSection(EncodingEmitter &emitter) {
 
   // Emit the referenced dialects.
   auto dialects = numberingState.getDialects();
   dialectEmitter.emitVarInt(llvm::size(dialects));
   for (DialectNumbering &dialect : dialects) {
     // Write the string section and get the ID.
     size_t nameID = stringSection.insert(dialect.name);
 
-    if (config.bytecodeVersion == 0) {
+    if (config.bytecodeVersion < bytecode::kDialectVersioning) {
       dialectEmitter.emitVarInt(nameID);
       continue;
     }
 
     // Try writing the version to the versionEmitter.
     EncodingEmitter versionEmitter;
     if (dialect.interface) {
       // The writer used when emitting using a custom bytecode encoding.
       DialectWriter versionWriter(config.bytecodeVersion, versionEmitter,
                                   numberingState, stringSection);
       dialect.interface->writeVersion(versionWriter);
     }
 
     // If the version emitter is empty, version is not available. We can encode
     // this in the dialect ID, so if there is no version, we don't write the
     // section.
     size_t versionAvailable = versionEmitter.size() > 0;
     dialectEmitter.emitVarIntWithFlag(nameID, versionAvailable);
     if (versionAvailable)
       dialectEmitter.emitSection(bytecode::Section::kDialectVersions,
                                  std::move(versionEmitter));
   }
 
-  if (config.bytecodeVersion > 3)
+  if (config.bytecodeVersion >= bytecode::kElideUnknownBlockArgLocation)
     dialectEmitter.emitVarInt(size(numberingState.getOpNames()));
 
   // Emit the referenced operation names grouped by dialect.
   auto emitOpName = [&](OpNameNumbering &name) {
     size_t stringId = stringSection.insert(name.name.stripDialect());
-    if (config.bytecodeVersion < 5)
+    if (config.bytecodeVersion < bytecode::kNativePropertiesEncoding)
       dialectEmitter.emitVarInt(stringId);
     else
       dialectEmitter.emitVarIntWithFlag(stringId, name.name.isRegistered());
   };
   writeDialectGrouping(dialectEmitter, numberingState.getOpNames(), emitOpName);
 
   emitter.emitSection(bytecode::Section::kDialect, std::move(dialectEmitter));
 }
LogicalResult BytecodeWriter::writeBlock(EncodingEmitter &emitter,
   unsigned numOps = numberingState.getOperationCount(block);
   emitter.emitVarIntWithFlag(numOps, hasArgs);
 
   // Emit the arguments of the block.
   if (hasArgs) {
     emitter.emitVarInt(args.size());
     for (BlockArgument arg : args) {
       Location argLoc = arg.getLoc();
-      if (config.bytecodeVersion > 3) {
+      if (config.bytecodeVersion >= bytecode::kElideUnknownBlockArgLocation) {
         emitter.emitVarIntWithFlag(numberingState.getNumber(arg.getType()),
                                    !isa<UnknownLoc>(argLoc));
         if (!isa<UnknownLoc>(argLoc))
           emitter.emitVarInt(numberingState.getNumber(argLoc));
       } else {
         emitter.emitVarInt(numberingState.getNumber(arg.getType()));
         emitter.emitVarInt(numberingState.getNumber(argLoc));
       }
     }
-    if (config.bytecodeVersion > 2) {
+    if (config.bytecodeVersion >= bytecode::kUseListOrdering) {
       uint64_t maskOffset = emitter.size();
       uint8_t encodingMask = 0;
       emitter.emitByte(0);
       writeUseListOrders(emitter, encodingMask, args);
       if (encodingMask)
         emitter.patchByte(maskOffset, encodingMask);
     }
   }
LogicalResult BytecodeWriter::writeOp(EncodingEmitter &emitter, Operation *op) {
   uint8_t opEncodingMask = 0;
   emitter.emitByte(0);
 
   // Emit the location for this operation.
   emitter.emitVarInt(numberingState.getNumber(op->getLoc()));
 
   // Emit the attributes of this operation.
   DictionaryAttr attrs = op->getDiscardableAttrDictionary();
-  // Allow deployment to version <5 by merging inherent attribute with the
-  // discardable ones. We should fail if there are any conflicts.
-  if (config.bytecodeVersion < 5)
+  // Allow deployment to version <kNativePropertiesEncoding by merging inherent
+  // attribute with the discardable ones. We should fail if there are any
+  // conflicts.
+  if (config.bytecodeVersion < bytecode::kNativePropertiesEncoding)
     attrs = op->getAttrDictionary();
   if (!attrs.empty()) {
     opEncodingMask |= bytecode::OpEncodingMask::kHasAttrs;
     emitter.emitVarInt(numberingState.getNumber(attrs));
   }
 
   // Emit the properties of this operation, for now we still support deployment
-  // to version <5.
-  if (config.bytecodeVersion >= 5) {
+  // to version <kNativePropertiesEncoding.
+  if (config.bytecodeVersion >= bytecode::kNativePropertiesEncoding) {
     std::optional<ssize_t> propertiesId = propertiesSection.emit(op);
     if (propertiesId.has_value()) {
       opEncodingMask |= bytecode::OpEncodingMask::kHasProperties;
       emitter.emitVarInt(*propertiesId);
     }
   }
 
   // Emit the result types of the operation.
if (unsigned numSuccessors = op->getNumSuccessors()) {
     opEncodingMask |= bytecode::OpEncodingMask::kHasSuccessors;
     emitter.emitVarInt(numSuccessors);
     for (Block *successor : op->getSuccessors())
       emitter.emitVarInt(numberingState.getNumber(successor));
   }
 
   // Emit the use-list orders to bytecode, so we can reconstruct the same order
   // at parsing.
-  if (config.bytecodeVersion > 2)
+  if (config.bytecodeVersion >= bytecode::kUseListOrdering)
     writeUseListOrders(emitter, opEncodingMask, ValueRange(op->getResults()));
 
   // Check for regions.
   unsigned numRegions = op->getNumRegions();
   if (numRegions)
     opEncodingMask |= bytecode::OpEncodingMask::kHasInlineRegions;
 
   // Update the mask for the operation.
   emitter.patchByte(maskOffset, opEncodingMask);
 
   // With the mask emitted, we can now emit the regions of the operation. We do
   // this after mask emission to avoid offset complications that may arise by
   // emitting the regions first (e.g. if the regions are huge, backpatching the
   // op encoding mask is more annoying).
   if (numRegions) {
     bool isIsolatedFromAbove = op->hasTrait<OpTrait::IsIsolatedFromAbove>();
     emitter.emitVarIntWithFlag(numRegions, isIsolatedFromAbove);
 
     for (Region &region : op->getRegions()) {
       // If the region is not isolated from above, or we are emitting bytecode
-      // targeting version <2, we don't use a section.
-      if (!isIsolatedFromAbove || config.bytecodeVersion < 2) {
+      // targeting version <kLazyLoading, we don't use a section.
+      if (!isIsolatedFromAbove ||
+          config.bytecodeVersion < bytecode::kLazyLoading) {
         if (failed(writeRegion(emitter, &region)))
           return failure();
         continue;
       }
 
       EncodingEmitter regionEmitter;
       if (failed(writeRegion(regionEmitter, &region)))
         return failure();