Fix MLIR bytecode loading of resources

[mehdi_amini]

Summary

The bytecode reader didn't handle properly the case where resource names
conflicted and were renamed, leading to orphan handles in the IR as well
as overwriting the exiting resources.

mlir/lib/AsmParser/Parser.cpp

 #include "mlir/IR/BuiltinOps.h"
 #include "mlir/IR/Dialect.h"
 #include "mlir/IR/Verifier.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/StringSet.h"
 #include "llvm/ADT/bit.h"
 #include "llvm/Support/Endian.h"
+#include "llvm/Support/MathExtras.h"
 #include "llvm/Support/PrettyStackTrace.h"
 #include "llvm/Support/SourceMgr.h"
 #include <algorithm>
 #include <optional>
 
 using namespace mlir;
 using namespace mlir::detail;
 
parseAsBlob(BlobAllocatorFn allocator) const final {
     // beginning of the string.
     if (blobData->size() < sizeof(uint32_t)) {
       return p.emitError(value.getLoc(),
                          "expected hex string blob for key '" + key +
                              "' to encode alignment in first 4 bytes");
     }
     llvm::support::ulittle32_t align;
     memcpy(&align, blobData->data(), sizeof(uint32_t));
+    if (align && !llvm::isPowerOf2_32(align)) {
+      return p.emitError(value.getLoc(),
+                         "expected hex string blob for key '" + key +
+                             "' to encode alignment in first 4 bytes, but got "
+                             "non-power-of-2 value: " +
+                             Twine(align));
+    }
 
     // Get the data portion of the blob.
     StringRef data = StringRef(*blobData).drop_front(sizeof(uint32_t));
     if (data.empty())
       return AsmResourceBlob();
 
     // Allocate memory for the blob using the provided allocator and copy the
     // data into it.

mlir/lib/Bytecode/Reader/BytecodeReader.cpp

 #include "mlir/IR/Verifier.h"
 #include "mlir/IR/Visitors.h"
 #include "mlir/Support/LLVM.h"
 #include "mlir/Support/LogicalResult.h"
 #include "llvm/ADT/MapVector.h"
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringExtras.h"
+#include "llvm/ADT/StringRef.h"
 #include "llvm/Support/MemoryBufferRef.h"
 #include "llvm/Support/SaveAndRestore.h"
 #include "llvm/Support/SourceMgr.h"
 #include <list>
 #include <memory>
 #include <numeric>
 #include <optional>
 
public:
   LogicalResult parseResourceHandle(EncodingReader &reader,
                                     AsmDialectResourceHandle &result) {
     return parseEntry(reader, dialectResources, result, "resource handle");
   }
 
 private:
   /// The table of dialect resources within the bytecode file.
   SmallVector<AsmDialectResourceHandle> dialectResources;
+  llvm::StringMap<std::string> dialectResourceHandleRenamingMap;
 };
 
 class ParsedResourceEntry : public AsmParsedResourceEntry {
 public:
   ParsedResourceEntry(StringRef key, AsmResourceEntryKind kind,
                       EncodingReader &reader, StringSectionReader &stringReader,
                       const std::shared_ptr<llvm::SourceMgr> &bufferOwnerRef)
       : key(key), kind(kind), reader(reader), stringReader(stringReader),

 } // namespace
 
 template <typename T>
 static LogicalResult
 parseResourceGroup(Location fileLoc, bool allowEmpty,
                    EncodingReader &offsetReader, EncodingReader &resourceReader,
                    StringSectionReader &stringReader, T *handler,
                    const std::shared_ptr<llvm::SourceMgr> &bufferOwnerRef,
+                   function_ref<StringRef(StringRef)> remapKey = {},
                    function_ref<LogicalResult(StringRef)> processKeyFn = {}) {
   uint64_t numResources;
   if (failed(offsetReader.parseVarInt(numResources)))
     return failure();
 
   for (uint64_t i = 0; i < numResources; ++i) {
     StringRef key;
     AsmResourceEntryKind kind;
if (allowEmpty && data.empty())
       continue;
 
     // Ignore the entry if we don't have a valid handler.
     if (!handler)
       continue;
 
     // Otherwise, parse the resource value.
     EncodingReader entryReader(data, fileLoc);
+    key = remapKey(key);
     ParsedResourceEntry entry(key, kind, entryReader, stringReader,
                               bufferOwnerRef);
     if (failed(handler->parseResource(entry)))
       return failure();
     if (!entryReader.empty()) {
       return entryReader.emitError(
           "unexpected trailing bytes in resource entry '", key, "'");
     }
LogicalResult ResourceSectionReader::initialize(
   uint64_t numExternalResourceGroups;
   if (failed(offsetReader.parseVarInt(numExternalResourceGroups)))
     return failure();
 
   // Utility functor that dispatches to `parseResourceGroup`, but implicitly
   // provides most of the arguments.
   auto parseGroup = [&](auto *handler, bool allowEmpty = false,
                         function_ref<LogicalResult(StringRef)> keyFn = {}) {
+    auto resolveKey = [&](StringRef key) -> StringRef {
+      auto it = dialectResourceHandleRenamingMap.find(key);
+      if (it == dialectResourceHandleRenamingMap.end())
+        return "";
+      return it->second;
+    };
+
     return parseResourceGroup(fileLoc, allowEmpty, offsetReader, resourceReader,
-                              stringReader, handler, bufferOwnerRef, keyFn);
+                              stringReader, handler, bufferOwnerRef, resolveKey,
+                              keyFn);
   };
 
   // Read the external resources from the bytecode.
   for (uint64_t i = 0; i < numExternalResourceGroups; ++i) {
     StringRef key;
     if (failed(stringReader.parseString(offsetReader, key)))
       return failure();
 
while (!offsetReader.empty()) {
     auto processResourceKeyFn = [&](StringRef key) -> LogicalResult {
       FailureOr<AsmDialectResourceHandle> handle =
           handler->declareResource(key);
       if (failed(handle)) {
         return resourceReader.emitError()
                << "unknown 'resource' key '" << key << "' for dialect '"
                << dialect->name << "'";
       }
+      dialectResourceHandleRenamingMap[key] = handler->getResourceKey(*handle);
       dialectResources.push_back(*handle);
       return success();
     };
 
     // Parse the resources for this dialect. We allow empty resources because we
     // just treat these as declarations.
     if (failed(parseGroup(handler, /*allowEmpty=*/true, processResourceKeyFn)))
       return failure();

mlir/unittests/Bytecode/BytecodeTest.cpp

+//===- AdaptorTest.cpp - Adaptor unit tests -------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "mlir/Bytecode/BytecodeReader.h"
+#include "mlir/Bytecode/BytecodeWriter.h"
+#include "mlir/IR/AsmState.h"
+#include "mlir/IR/BuiltinAttributes.h"
+#include "mlir/IR/OpImplementation.h"
+#include "mlir/IR/OwningOpRef.h"
+#include "mlir/Parser/Parser.h"
+
+#include "llvm/ADT/StringRef.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+using namespace mlir;
+
+using testing::ElementsAre;
+
+StringLiteral IRWithResources = R"(
+module @TestDialectResources attributes {
+  bytecode.test = dense_resource<resource> : tensor<4xi32>
+} {}
+{-#
+  dialect_resources: {
+    builtin: {
+      resource: "0x1000000001000000020000000300000004000000"
+    }
+  }
+#-}
+)";
+
+TEST(Bytecode, MultiModuleWithResource) {
+  MLIRContext context;
+  Builder builder(&context);
+  ParserConfig parseConfig(&context);
+  OwningOpRef<Operation *> module =
+      parseSourceString<Operation *>(IRWithResources, parseConfig);
+  ASSERT_TRUE(module);
+
+  // Write the module to bytecode
+  std::string buffer;
+  llvm::raw_string_ostream ostream(buffer);
+  ASSERT_TRUE(succeeded(writeBytecodeToFile(module.get(), ostream)));
+
+  // Parse it back
+  OwningOpRef<Operation *> roundTripModule =
+      parseSourceString<Operation *>(ostream.str(), parseConfig);
+  ASSERT_TRUE(roundTripModule);
+
+  // Try to see if we have a valid resource in the parsed module.
+  auto checkResourceAttribute = [&](Operation *op) {
+    Attribute attr = roundTripModule->getAttr("bytecode.test");
+    ASSERT_TRUE(attr);
+    auto denseResourceAttr = dyn_cast<DenseI32ResourceElementsAttr>(attr);
+    ASSERT_TRUE(denseResourceAttr);
+    std::optional<ArrayRef<int32_t>> attrData =
+        denseResourceAttr.tryGetAsArrayRef();
+    ASSERT_TRUE(attrData.has_value());
+    ASSERT_EQ(attrData->size(), static_cast<size_t>(4));
+    EXPECT_EQ((*attrData)[0], 1);
+    EXPECT_EQ((*attrData)[1], 2);
+    EXPECT_EQ((*attrData)[2], 3);
+    EXPECT_EQ((*attrData)[3], 4);
+  };
+
+  checkResourceAttribute(*module);
+  checkResourceAttribute(*roundTripModule);
+}

mlir/unittests/Bytecode/CMakeLists.txt

+add_mlir_unittest(MLIRBytecodeTests
+  BytecodeTest.cpp
+)
+target_link_libraries(MLIRBytecodeTests
+  PRIVATE
+  MLIRBytecodeReader
+  MLIRBytecodeWriter
+  MLIRParser
+)

mlir/unittests/CMakeLists.txt

 set_target_properties(MLIRUnitTests PROPERTIES FOLDER "MLIR Tests")
 
 function(add_mlir_unittest test_dirname)
   add_unittest(MLIRUnitTests ${test_dirname} ${ARGN})
 endfunction()
 
 add_subdirectory(Analysis)
+add_subdirectory(Bytecode)
 add_subdirectory(Conversion)
 add_subdirectory(Debug)
 add_subdirectory(Dialect)
 add_subdirectory(Interfaces)
 add_subdirectory(IR)
 add_subdirectory(Parser)
 add_subdirectory(Pass)
 add_subdirectory(Support)
 add_subdirectory(Rewrite)
 add_subdirectory(TableGen)
 add_subdirectory(Transforms)
 
 if(MLIR_ENABLE_EXECUTION_ENGINE)
   add_subdirectory(ExecutionEngine)
 endif()