-
Notifications
You must be signed in to change notification settings - Fork 14.9k
Revert "[MLIR] Fix duplicated attribute nodes in MLIR bytecode deserialization (#151267) #155214
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@llvm/pr-subscribers-mlir @llvm/pr-subscribers-mlir-core Author: Christian Ulmann (Dinistro) ChangesThis 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:
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
The commit description isn’t super clear: what kind of unwanted collision? Is there a test case added to protect against regressions? |
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? |
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). |
This reverts commit c075fb8. This commit introduces a caching bug that causes undesired collisions.