Skip to content

[mlir][bytecode] Avoid crash when test dialect version is missing #170173

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.

Sign up for GitHub

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

Open
Men-cotton wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[mlir][bytecode] Avoid crash when test dialect version is missing #170173

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions mlir/test/Bytecode/roundtrip-missing-dialect.mlir
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
// RUN: mlir-opt %s --test-bytecode-roundtrip=test-dialect-version=2.0 | FileCheck %s

// CHECK-LABEL: func.func @main
func.func @main() {
return
}
8 changes: 4 additions & 4 deletions mlir/test/lib/IR/TestBytecodeRoundtrip.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,8 +141,8 @@ struct TestBytecodeRoundtripPass
DialectBytecodeWriter &writer) -> LogicalResult {
// Do not override anything if version greater than 2.0.
auto versionOr = writer.getDialectVersion<test::TestDialect>();
assert(succeeded(versionOr) && "expected reader to be able to access "
"the version for test dialect");
if (failed(versionOr))
return failure();
const auto *version =
reinterpret_cast<const test::TestDialectVersion *>(*versionOr);
if (version->major_ >= 2)
Expand All @@ -166,8 +166,8 @@ struct TestBytecodeRoundtripPass
Type &entry) -> LogicalResult {
// Get test dialect version from the version map.
auto versionOr = reader.getDialectVersion<test::TestDialect>();
assert(succeeded(versionOr) && "expected reader to be able to access "
"the version for test dialect");
if (failed(versionOr))
return success();
Copy link
Member

@ftynse ftynse Dec 5, 2025

Choose a reason for hiding this comment

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

Should this be failure?

Copy link
Contributor Author

@Men-cotton Men-cotton Dec 6, 2025

Choose a reason for hiding this comment

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

Thanks for the comment!
I’d prefer to keep this as return success(); rather than failure().

My understanding is that for the Reader callbacks, returning failure() indicates a hard error and aborts the reading process immediately, whereas returning success() with entry left unset means “I didn’t handle this, please fall back to the default path”.

Since the goal here is to gracefully fallback (skip this callback) when the version is missing, I believe return success() is the correct return value for the Reader side (in contrast to the Writer side, where failure() triggers a fallback). This also matches the overall design goal described in this comment, that bytecode handling should be robust and not fail in such cases.

If I’ve misunderstood the intended reader contract, I’m happy to adjust.

const auto *version =
reinterpret_cast<const test::TestDialectVersion *>(*versionOr);
if (version->major_ >= 2)
Expand Down