Skip to content

Conversation

Dinistro
Copy link
Contributor

This reverts commit c075fb8. This commit introduces a caching bug that causes undesired collisions.

…alization (#151267)"

This reverts commit c075fb8. This
commit introduces a caching bug that causes undesired collisions.
@llvmbot
Copy link
Member

llvmbot commented Aug 25, 2025

@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-mlir-core

Author: Christian Ulmann (Dinistro)

Changes

This reverts commit c075fb8. This commit introduces a caching bug that causes undesired collisions.


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

5 Files Affected:

  • (modified) mlir/include/mlir/AsmParser/AsmParser.h (+1-2)
  • (modified) mlir/lib/AsmParser/DialectSymbolParser.cpp (+2-22)
  • (modified) mlir/lib/AsmParser/ParserState.h (-3)
  • (modified) mlir/lib/Bytecode/Reader/BytecodeReader.cpp (+1-5)
  • (removed) mlir/test/IR/recursive-distinct-attr.mlir (-13)
diff --git a/mlir/include/mlir/AsmParser/AsmParser.h b/mlir/include/mlir/AsmParser/AsmParser.h
index f39b3bd853a2a..33daf7ca26f49 100644
--- a/mlir/include/mlir/AsmParser/AsmParser.h
+++ b/mlir/include/mlir/AsmParser/AsmParser.h
@@ -53,8 +53,7 @@ parseAsmSourceFile(const llvm::SourceMgr &sourceMgr, Block *block,
 /// null terminated.
 Attribute parseAttribute(llvm::StringRef attrStr, MLIRContext *context,
                          Type type = {}, size_t *numRead = nullptr,
-                         bool isKnownNullTerminated = false,
-                         llvm::StringMap<Attribute> *attributesCache = nullptr);
+                         bool isKnownNullTerminated = false);
 
 /// This parses a single MLIR type to an MLIR context if it was valid. If not,
 /// an error diagnostic is emitted to the context.
diff --git a/mlir/lib/AsmParser/DialectSymbolParser.cpp b/mlir/lib/AsmParser/DialectSymbolParser.cpp
index 416d8eb5f40e0..8b14e71118c3a 100644
--- a/mlir/lib/AsmParser/DialectSymbolParser.cpp
+++ b/mlir/lib/AsmParser/DialectSymbolParser.cpp
@@ -245,15 +245,6 @@ static Symbol parseExtendedSymbol(Parser &p, AsmParserState *asmState,
       return nullptr;
   }
 
-  if constexpr (std::is_same_v<Symbol, Attribute>) {
-    auto &cache = p.getState().symbols.attributesCache;
-    auto cacheIt = cache.find(symbolData);
-    // Skip cached attribute if it has type.
-    if (cacheIt != cache.end() && !p.getToken().is(Token::colon))
-      return cacheIt->second;
-
-    return cache[symbolData] = createSymbol(dialectName, symbolData, loc);
-  }
   return createSymbol(dialectName, symbolData, loc);
 }
 
@@ -346,7 +337,6 @@ Type Parser::parseExtendedType() {
 template <typename T, typename ParserFn>
 static T parseSymbol(StringRef inputStr, MLIRContext *context,
                      size_t *numReadOut, bool isKnownNullTerminated,
-                     llvm::StringMap<Attribute> *attributesCache,
                      ParserFn &&parserFn) {
   // Set the buffer name to the string being parsed, so that it appears in error
   // diagnostics.
@@ -358,9 +348,6 @@ static T parseSymbol(StringRef inputStr, MLIRContext *context,
   SourceMgr sourceMgr;
   sourceMgr.AddNewSourceBuffer(std::move(memBuffer), SMLoc());
   SymbolState aliasState;
-  if (attributesCache)
-    aliasState.attributesCache = *attributesCache;
-
   ParserConfig config(context);
   ParserState state(sourceMgr, config, aliasState, /*asmState=*/nullptr,
                     /*codeCompleteContext=*/nullptr);
@@ -371,11 +358,6 @@ static T parseSymbol(StringRef inputStr, MLIRContext *context,
   if (!symbol)
     return T();
 
-  if constexpr (std::is_same_v<T, Attribute>) {
-    if (attributesCache)
-      *attributesCache = state.symbols.attributesCache;
-  }
-
   // Provide the number of bytes that were read.
   Token endTok = parser.getToken();
   size_t numRead =
@@ -392,15 +374,13 @@ static T parseSymbol(StringRef inputStr, MLIRContext *context,
 
 Attribute mlir::parseAttribute(StringRef attrStr, MLIRContext *context,
                                Type type, size_t *numRead,
-                               bool isKnownNullTerminated,
-                               llvm::StringMap<Attribute> *attributesCache) {
+                               bool isKnownNullTerminated) {
   return parseSymbol<Attribute>(
-      attrStr, context, numRead, isKnownNullTerminated, attributesCache,
+      attrStr, context, numRead, isKnownNullTerminated,
       [type](Parser &parser) { return parser.parseAttribute(type); });
 }
 Type mlir::parseType(StringRef typeStr, MLIRContext *context, size_t *numRead,
                      bool isKnownNullTerminated) {
   return parseSymbol<Type>(typeStr, context, numRead, isKnownNullTerminated,
-                           /*attributesCache=*/nullptr,
                            [](Parser &parser) { return parser.parseType(); });
 }
diff --git a/mlir/lib/AsmParser/ParserState.h b/mlir/lib/AsmParser/ParserState.h
index aa53032107cbf..159058a18fa4e 100644
--- a/mlir/lib/AsmParser/ParserState.h
+++ b/mlir/lib/AsmParser/ParserState.h
@@ -40,9 +40,6 @@ struct SymbolState {
 
   /// A map from unique integer identifier to DistinctAttr.
   DenseMap<uint64_t, DistinctAttr> distinctAttributes;
-
-  /// A map from unique string identifier to Attribute.
-  llvm::StringMap<Attribute> attributesCache;
 };
 
 //===----------------------------------------------------------------------===//
diff --git a/mlir/lib/Bytecode/Reader/BytecodeReader.cpp b/mlir/lib/Bytecode/Reader/BytecodeReader.cpp
index 0f97443433774..44458d010c6c8 100644
--- a/mlir/lib/Bytecode/Reader/BytecodeReader.cpp
+++ b/mlir/lib/Bytecode/Reader/BytecodeReader.cpp
@@ -895,10 +895,6 @@ class AttrTypeReader {
   SmallVector<AttrEntry> attributes;
   SmallVector<TypeEntry> types;
 
-  /// The map of cached attributes, used to avoid re-parsing the same
-  /// attribute multiple times.
-  llvm::StringMap<Attribute> attributesCache;
-
   /// A location used for error emission.
   Location fileLoc;
 
@@ -1239,7 +1235,7 @@ LogicalResult AttrTypeReader::parseAsmEntry(T &result, EncodingReader &reader,
         ::parseType(asmStr, context, &numRead, /*isKnownNullTerminated=*/true);
   else
     result = ::parseAttribute(asmStr, context, Type(), &numRead,
-                              /*isKnownNullTerminated=*/true, &attributesCache);
+                              /*isKnownNullTerminated=*/true);
   if (!result)
     return failure();
 
diff --git a/mlir/test/IR/recursive-distinct-attr.mlir b/mlir/test/IR/recursive-distinct-attr.mlir
deleted file mode 100644
index 5afb5c59e0fcf..0000000000000
--- a/mlir/test/IR/recursive-distinct-attr.mlir
+++ /dev/null
@@ -1,13 +0,0 @@
-// RUN: mlir-opt -emit-bytecode %s | mlir-opt --mlir-print-debuginfo | FileCheck %s
-
-// Verify that the distinct attribute which is used transitively
-// through two aliases does not end up duplicated when round-tripped
-// through bytecode.
-
-// CHECK: distinct[0]
-// CHECK-NOT: distinct[1]
-#attr_ugly = #test<attr_ugly begin distinct[0]<> end>
-#attr_ugly1 = #test<attr_ugly begin #attr_ugly end>
-
-module attributes {test.alias = #attr_ugly, test.alias1 = #attr_ugly1} {
-}
\ No newline at end of file

Copy link
Contributor

@gysit gysit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Dinistro Dinistro merged commit 58953b8 into main Aug 25, 2025
12 checks passed
@Dinistro Dinistro deleted the users/dinistro/bytecode-bug branch August 25, 2025 11:46
@joker-eph
Copy link
Collaborator

joker-eph commented Aug 25, 2025

The commit description isn’t super clear: what kind of unwanted collision? Is there a test case added to protect against regressions?
if not, can you please add one?

@Dinistro
Copy link
Contributor Author

Dinistro commented Aug 25, 2025

I reverted because this breaks multiple down-stream projects. There is a reproducer in the linked ticket that was opened: #155210

Isn't the policy to revert if a breakage has been spotted and it's on the author to address issues and reland?

@joker-eph
Copy link
Collaborator

joker-eph commented Aug 25, 2025

I'm not questioning the revert itself: fast reverts are always appreciated, thanks :)

I opened this PR and didn't see a link to the #155210), so I just pointed out the incompleteness of the commit message here (missing context).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mlir:core MLIR Core Infrastructure mlir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants