[mlir][BytecodeReader] Fix FusedLoc bytecode builder #99632
Conversation
|
@llvm/pr-subscribers-mlir-ods @llvm/pr-subscribers-mlir-core Author: Hideto Ueno (uenoku) Changes
Fix #99626. Full diff: https://github.com/llvm/llvm-project/pull/99632.diff 2 Files Affected:
diff --git a/mlir/include/mlir/IR/BuiltinDialectBytecode.td b/mlir/include/mlir/IR/BuiltinDialectBytecode.td
index f50b5dd7ad822..0062bcbf6ea11 100644
--- a/mlir/include/mlir/IR/BuiltinDialectBytecode.td
+++ b/mlir/include/mlir/IR/BuiltinDialectBytecode.td
@@ -101,12 +101,12 @@ def FileLineColLoc : DialectAttribute<(attr
VarInt:$column
)>;
-let cType = "FusedLoc",
- cBuilder = "cast<FusedLoc>(get<FusedLoc>(context, $_args))" in {
+let cType = "FusedLoc" in {
def FusedLoc : DialectAttribute<(attr
Array<Location>:$locations
)> {
let printerPredicate = "!$_val.getMetadata()";
+ let cBuilder = "cast<FusedLoc>(get<FusedLoc>(context, $_args, Attribute()))";
}
def FusedLocWithMetadata : DialectAttribute<(attr
@@ -114,6 +114,7 @@ def FusedLocWithMetadata : DialectAttribute<(attr
Attribute:$metadata
)> {
let printerPredicate = "$_val.getMetadata()";
+ let cBuilder = "cast<FusedLoc>(get<FusedLoc>(context, $_args))";
}
}
diff --git a/mlir/unittests/Bytecode/BytecodeTest.cpp b/mlir/unittests/Bytecode/BytecodeTest.cpp
index a37a2afc22645..43649819057d0 100644
--- a/mlir/unittests/Bytecode/BytecodeTest.cpp
+++ b/mlir/unittests/Bytecode/BytecodeTest.cpp
@@ -10,6 +10,7 @@
#include "mlir/Bytecode/BytecodeWriter.h"
#include "mlir/IR/AsmState.h"
#include "mlir/IR/BuiltinAttributes.h"
+#include "mlir/IR/BuiltinOps.h"
#include "mlir/IR/OpImplementation.h"
#include "mlir/IR/OwningOpRef.h"
#include "mlir/Parser/Parser.h"
@@ -148,3 +149,24 @@ TEST(Bytecode, OpWithoutProperties) {
EXPECT_TRUE(OperationEquivalence::computeHash(op.get()) ==
OperationEquivalence::computeHash(roundtripped));
}
+
+TEST(Bytecode, FusedLocCrash) {
+ MLIRContext context;
+ OpBuilder builder(&context);
+ SmallVector<Location> locs;
+ FusedLoc fusedLoc = FusedLoc::get(&context, locs, {});
+
+ auto module = builder.create<mlir::ModuleOp>(fusedLoc, "test");
+
+ // Write the module to bytecode
+ std::string buffer;
+ llvm::raw_string_ostream ostream(buffer);
+ ASSERT_TRUE(succeeded(writeBytecodeToFile(module, ostream)));
+ ostream.flush();
+
+ // Parse it back
+ ParserConfig parseConfig(&context);
+ OwningOpRef<Operation *> roundTripModule =
+ parseSourceString<Operation *>(buffer, parseConfig);
+ ASSERT_TRUE(roundTripModule);
+}
|
| OwningOpRef<Operation *> roundTripModule = | ||
| parseSourceString<Operation *>(buffer, parseConfig); | ||
| ASSERT_TRUE(roundTripModule); | ||
| } |
There was a problem hiding this comment.
Can you write a lit test running with mlir-opt for this instead?
There was a problem hiding this comment.
I think this issue is not reproducible from mlir textual format since fused[] is converted into unknown in the asm parser. What do you think?
$ cat bar.mlir
module @TestLocFused {
} loc(#loc)
#loc = loc(fused[])
$ mlir-opt bar.mlir -mlir-print-debuginfo
module @TestLocFused {
} loc(#loc)
#loc = loc(unknown)There was a problem hiding this comment.
The in-memory IR should round-trip to both bytecode and text: if it isn't the case right now we need to fix both the bytecode and textual printer/parser.
There was a problem hiding this comment.
Textual printer/parser are working fine since FusedLoc is converted into Unknown in the parser. I think the bug is only in BytecodeReader.
| MLIRContext context; | ||
| OpBuilder builder(&context); | ||
| SmallVector<Location> locs; | ||
| FusedLoc fusedLoc = FusedLoc::get(&context, locs, {}); |
There was a problem hiding this comment.
Should we disallow this at the constructor? It feels weird to have a fused location with no locations. I don't know when one would actually want this behavior to be valid.
There was a problem hiding this comment.
Disallowing empty location looks reasonable but the issue itself is not specific to empty location. We should be able to get the same failure when fused loc has a single location (this is the code) e.g:
FileLineColLoc loc = ...;
FusedLoc fusedLoc = FusedLoc::get(&context, {loc}, {}) 
How do you even get in this situation? And is intentional? (UnknownLoc wouldn't support FusedLoc metadata, so this isn't a lossless result of the builder). |
CIRCT has a pass that drops specific locations (basically same as |
FusedLoc::get(MLIRContext*, ArrayRef<Location>)is not guaranteed to returnFusedLocbut it is used when reading bytecode. This PR changes to use a different getterFusedLoc::get(MLIRContext*, ArrayRef<Location>, Attribute attribute)which always returnsFusedLoc.Fix #99626.