[mlir][Func] Fix FuncOp verifier ordering via hasRegionVerifier#184612
Merged
Conversation
Member
|
@llvm/pr-subscribers-mlir Author: Mehdi Amini (joker-eph) ChangesFuncOp::verify() iterated over all blocks and called getMutableSuccessorOperands() on any RegionBranchTerminatorOpInterface terminator to check return types. This ran during the entrance phase of verification — before child ops had been verified — so a malformed terminator whose getMutableSuccessorOperands() assumed invariants established by its own verify() could crash instead of emitting a clean diagnostic. Fix by switching to hasRegionVerifier=1: rename verify() → verifyRegions() so the return-type checks run in the exit phase, after all nested ops have already been verified. To demonstrate the bug and guard against regression, add TestCrashingReturnOp to the test dialect. The op implements RegionBranchTerminatorOpInterface and report_fatal_errors in getMutableSuccessorOperands() when its 'valid' unit-attr is absent, reproducing the class of crash described above. The accompanying lit test confirms a clean diagnostic is emitted rather than a crash. Full diff: https://github.com/llvm/llvm-project/pull/184612.diff 5 Files Affected:
diff --git a/mlir/include/mlir/Dialect/Func/IR/FuncOps.td b/mlir/include/mlir/Dialect/Func/IR/FuncOps.td
index ce2b7228cb954..a31b860276099 100644
--- a/mlir/include/mlir/Dialect/Func/IR/FuncOps.td
+++ b/mlir/include/mlir/Dialect/Func/IR/FuncOps.td
@@ -357,7 +357,7 @@ def FuncOp : Func_Op<"func", [
bool isDeclaration() { return isExternal(); }
}];
let hasCustomAssemblyFormat = 1;
- let hasVerifier = 1;
+ let hasRegionVerifier = 1;
}
//===----------------------------------------------------------------------===//
diff --git a/mlir/lib/Dialect/Func/IR/FuncOps.cpp b/mlir/lib/Dialect/Func/IR/FuncOps.cpp
index 2749ad5262f2b..d803e99154499 100644
--- a/mlir/lib/Dialect/Func/IR/FuncOps.cpp
+++ b/mlir/lib/Dialect/Func/IR/FuncOps.cpp
@@ -284,7 +284,7 @@ FuncOp FuncOp::clone() {
// ReturnOp
//===----------------------------------------------------------------------===//
-LogicalResult FuncOp::verify() {
+LogicalResult FuncOp::verifyRegions() {
// External declarations have no body to check.
if (isDeclaration())
return success();
diff --git a/mlir/test/Dialect/Func/func-verifier-order-crash.mlir b/mlir/test/Dialect/Func/func-verifier-order-crash.mlir
new file mode 100644
index 0000000000000..7896f7d37c63f
--- /dev/null
+++ b/mlir/test/Dialect/Func/func-verifier-order-crash.mlir
@@ -0,0 +1,12 @@
+// After moving the return-type checks from FuncOp::verify() to
+// FuncOp::verifyRegions() (hasRegionVerifier=1), the terminator is verified
+// first and emits a clean diagnostic instead of crashing.
+//
+// RUN: mlir-opt %s -split-input-file -verify-diagnostics
+
+// -----
+
+func.func @func_verifier_runs_before_region() -> () {
+ // expected-error @below {{requires 'valid' unit attribute}}
+ test.crashing_return
+}
diff --git a/mlir/test/lib/Dialect/Test/TestOpDefs.cpp b/mlir/test/lib/Dialect/Test/TestOpDefs.cpp
index c243bd79a44a8..7cf728f933395 100644
--- a/mlir/test/lib/Dialect/Test/TestOpDefs.cpp
+++ b/mlir/test/lib/Dialect/Test/TestOpDefs.cpp
@@ -1287,6 +1287,24 @@ LoopBlockTerminatorOp::getMutableSuccessorOperands(RegionSuccessor successor) {
return getNextIterArgMutable();
}
+//===----------------------------------------------------------------------===//
+// TestCrashingReturnOp
+//===----------------------------------------------------------------------===//
+
+LogicalResult TestCrashingReturnOp::verify() {
+ if (!getValid())
+ return emitOpError("requires 'valid' unit attribute");
+ return success();
+}
+
+MutableOperandRange
+TestCrashingReturnOp::getMutableSuccessorOperands(RegionSuccessor successor) {
+ if (!getValid())
+ llvm::report_fatal_error("getMutableSuccessorOperands called on unverified "
+ "test.crashing_return");
+ return getArgsMutable();
+}
+
//===----------------------------------------------------------------------===//
// SwitchWithNoBreakOp
//===----------------------------------------------------------------------===//
diff --git a/mlir/test/lib/Dialect/Test/TestOps.td b/mlir/test/lib/Dialect/Test/TestOps.td
index fe02536a1df5b..bd0b6e25efa53 100644
--- a/mlir/test/lib/Dialect/Test/TestOps.td
+++ b/mlir/test/lib/Dialect/Test/TestOps.td
@@ -2239,6 +2239,17 @@ def TestReturnOp : TEST_Op<"return", [Pure, ReturnLike, Terminator]> {
[{ build($_builder, $_state, {}); }]>
];
}
+// A return-like terminator used to test FuncOp verifier ordering.
+// When 'valid' unit-attr is absent the op is malformed; its verifier
+// emits an error, but getMutableSuccessorOperands() calls report_fatal_error
+// to expose the fact that FuncOp::verify() runs before the region is checked.
+def TestCrashingReturnOp : TEST_Op<"crashing_return", [
+ DeclareOpInterfaceMethods<RegionBranchTerminatorOpInterface>,
+ Terminator]> {
+ let arguments = (ins Variadic<AnyType>:$args, UnitAttr:$valid);
+ let assemblyFormat = "($args^ `:` type($args))? attr-dict";
+ let hasVerifier = 1;
+}
def TestCastOp : TEST_Op<"cast">,
Arguments<(ins Variadic<AnyType>)>, Results<(outs AnyType)>;
def TestInvalidOp : TEST_Op<"invalid", [Terminator]>,
|
Member
|
@llvm/pr-subscribers-mlir-func Author: Mehdi Amini (joker-eph) ChangesFuncOp::verify() iterated over all blocks and called getMutableSuccessorOperands() on any RegionBranchTerminatorOpInterface terminator to check return types. This ran during the entrance phase of verification — before child ops had been verified — so a malformed terminator whose getMutableSuccessorOperands() assumed invariants established by its own verify() could crash instead of emitting a clean diagnostic. Fix by switching to hasRegionVerifier=1: rename verify() → verifyRegions() so the return-type checks run in the exit phase, after all nested ops have already been verified. To demonstrate the bug and guard against regression, add TestCrashingReturnOp to the test dialect. The op implements RegionBranchTerminatorOpInterface and report_fatal_errors in getMutableSuccessorOperands() when its 'valid' unit-attr is absent, reproducing the class of crash described above. The accompanying lit test confirms a clean diagnostic is emitted rather than a crash. Full diff: https://github.com/llvm/llvm-project/pull/184612.diff 5 Files Affected:
diff --git a/mlir/include/mlir/Dialect/Func/IR/FuncOps.td b/mlir/include/mlir/Dialect/Func/IR/FuncOps.td
index ce2b7228cb954..a31b860276099 100644
--- a/mlir/include/mlir/Dialect/Func/IR/FuncOps.td
+++ b/mlir/include/mlir/Dialect/Func/IR/FuncOps.td
@@ -357,7 +357,7 @@ def FuncOp : Func_Op<"func", [
bool isDeclaration() { return isExternal(); }
}];
let hasCustomAssemblyFormat = 1;
- let hasVerifier = 1;
+ let hasRegionVerifier = 1;
}
//===----------------------------------------------------------------------===//
diff --git a/mlir/lib/Dialect/Func/IR/FuncOps.cpp b/mlir/lib/Dialect/Func/IR/FuncOps.cpp
index 2749ad5262f2b..d803e99154499 100644
--- a/mlir/lib/Dialect/Func/IR/FuncOps.cpp
+++ b/mlir/lib/Dialect/Func/IR/FuncOps.cpp
@@ -284,7 +284,7 @@ FuncOp FuncOp::clone() {
// ReturnOp
//===----------------------------------------------------------------------===//
-LogicalResult FuncOp::verify() {
+LogicalResult FuncOp::verifyRegions() {
// External declarations have no body to check.
if (isDeclaration())
return success();
diff --git a/mlir/test/Dialect/Func/func-verifier-order-crash.mlir b/mlir/test/Dialect/Func/func-verifier-order-crash.mlir
new file mode 100644
index 0000000000000..7896f7d37c63f
--- /dev/null
+++ b/mlir/test/Dialect/Func/func-verifier-order-crash.mlir
@@ -0,0 +1,12 @@
+// After moving the return-type checks from FuncOp::verify() to
+// FuncOp::verifyRegions() (hasRegionVerifier=1), the terminator is verified
+// first and emits a clean diagnostic instead of crashing.
+//
+// RUN: mlir-opt %s -split-input-file -verify-diagnostics
+
+// -----
+
+func.func @func_verifier_runs_before_region() -> () {
+ // expected-error @below {{requires 'valid' unit attribute}}
+ test.crashing_return
+}
diff --git a/mlir/test/lib/Dialect/Test/TestOpDefs.cpp b/mlir/test/lib/Dialect/Test/TestOpDefs.cpp
index c243bd79a44a8..7cf728f933395 100644
--- a/mlir/test/lib/Dialect/Test/TestOpDefs.cpp
+++ b/mlir/test/lib/Dialect/Test/TestOpDefs.cpp
@@ -1287,6 +1287,24 @@ LoopBlockTerminatorOp::getMutableSuccessorOperands(RegionSuccessor successor) {
return getNextIterArgMutable();
}
+//===----------------------------------------------------------------------===//
+// TestCrashingReturnOp
+//===----------------------------------------------------------------------===//
+
+LogicalResult TestCrashingReturnOp::verify() {
+ if (!getValid())
+ return emitOpError("requires 'valid' unit attribute");
+ return success();
+}
+
+MutableOperandRange
+TestCrashingReturnOp::getMutableSuccessorOperands(RegionSuccessor successor) {
+ if (!getValid())
+ llvm::report_fatal_error("getMutableSuccessorOperands called on unverified "
+ "test.crashing_return");
+ return getArgsMutable();
+}
+
//===----------------------------------------------------------------------===//
// SwitchWithNoBreakOp
//===----------------------------------------------------------------------===//
diff --git a/mlir/test/lib/Dialect/Test/TestOps.td b/mlir/test/lib/Dialect/Test/TestOps.td
index fe02536a1df5b..bd0b6e25efa53 100644
--- a/mlir/test/lib/Dialect/Test/TestOps.td
+++ b/mlir/test/lib/Dialect/Test/TestOps.td
@@ -2239,6 +2239,17 @@ def TestReturnOp : TEST_Op<"return", [Pure, ReturnLike, Terminator]> {
[{ build($_builder, $_state, {}); }]>
];
}
+// A return-like terminator used to test FuncOp verifier ordering.
+// When 'valid' unit-attr is absent the op is malformed; its verifier
+// emits an error, but getMutableSuccessorOperands() calls report_fatal_error
+// to expose the fact that FuncOp::verify() runs before the region is checked.
+def TestCrashingReturnOp : TEST_Op<"crashing_return", [
+ DeclareOpInterfaceMethods<RegionBranchTerminatorOpInterface>,
+ Terminator]> {
+ let arguments = (ins Variadic<AnyType>:$args, UnitAttr:$valid);
+ let assemblyFormat = "($args^ `:` type($args))? attr-dict";
+ let hasVerifier = 1;
+}
def TestCastOp : TEST_Op<"cast">,
Arguments<(ins Variadic<AnyType>)>, Results<(outs AnyType)>;
def TestInvalidOp : TEST_Op<"invalid", [Terminator]>,
|
Contributor
|
Nice! Thanks a lot for the quick fix! It'll take me time to test it against my problematic test but from the looks I expect it to work -- thanks :) |
ingomueller-net
approved these changes
Mar 4, 2026
matthias-springer
approved these changes
Mar 4, 2026
FuncOp::verify() iterated over all blocks and called getMutableSuccessorOperands() on any RegionBranchTerminatorOpInterface terminator to check return types. This ran during the entrance phase of verification — before child ops had been verified — so a malformed terminator whose getMutableSuccessorOperands() assumed invariants established by its own verify() could crash instead of emitting a clean diagnostic. Fix by switching to hasRegionVerifier=1: rename verify() → verifyRegions() so the return-type checks run in the exit phase, after all nested ops have already been verified. To demonstrate the bug and guard against regression, add TestCrashingReturnOp to the test dialect. The op implements RegionBranchTerminatorOpInterface and report_fatal_errors in getMutableSuccessorOperands() when its 'valid' unit-attr is absent, reproducing the class of crash described above. The accompanying lit test confirms a clean diagnostic is emitted rather than a crash.
joker-eph
force-pushed
the
fix_func_verification
branch
from
a736daf to
edfaff8
Compare
sujianIBM
pushed a commit
to sujianIBM/llvm-project
that referenced
this pull request
Mar 5, 2026
…#184612) FuncOp::verify() iterated over all blocks and called getMutableSuccessorOperands() on any RegionBranchTerminatorOpInterface terminator to check return types. This ran during the entrance phase of verification — before child ops had been verified — so a malformed terminator whose getMutableSuccessorOperands() assumed invariants established by its own verify() could crash instead of emitting a clean diagnostic. Fix by switching to hasRegionVerifier=1: rename verify() → verifyRegions() so the return-type checks run in the exit phase, after all nested ops have already been verified. To demonstrate the bug and guard against regression, add TestCrashingReturnOp to the test dialect. The op implements RegionBranchTerminatorOpInterface and report_fatal_errors in getMutableSuccessorOperands() when its 'valid' unit-attr is absent, reproducing the class of crash described above. The accompanying lit test confirms a clean diagnostic is emitted rather than a crash.
modularbot
pushed a commit
to modular/modular
that referenced
this pull request
Mar 7, 2026
Changes: - Renamed MLIR pass registration functions to include `Pass` suffix (`registerCanonicalizerPass`, `registerStripDebugInfoPass`, etc.) following upstream [785490e9db54](llvm/llvm-project@785490e) ([#183950](llvm/llvm-project#183950)) - Updated `llvm-lldb-exports.patch` for upstream LLDBLog refactoring: - `InitializeLldbChannel` renamed to `InitializeLLDBChannel`/`TerminateLLDBChannel` ([d4d18248](llvm/llvm-project@d4d18248fde6)) - Functions then wrapped in `LLDBLogChannel` class ([45dbce3a](llvm/llvm-project@45dbce3a3a3e)) - Fix `executeFromDriver` to accept non-const `SmallVectorImpl` after `MutableArrayRef` stopped accepting const container sources ([c88ba88d](llvm/llvm-project@c88ba88da52b)) - Fix MGP dialect tests: add return types to `func.func` declarations now that MLIR's `FuncOp` verifier checks `ReturnLike` ops (including `mef.output`) against declared return types, following upstream [b28ec5ad1808](llvm/llvm-project@b28ec5a) ([#184612](llvm/llvm-project#184612)) - Fix GCN FileCheck patterns for AMDGPU `@llvm.amdgcn.implicitarg.ptr()` now emitting `dereferenceable(256)` return attribute, following upstream [fdc4a982f5d6](llvm/llvm-project@fdc4a982f5d6) ([#182206](llvm/llvm-project#182206)) - Update GCN shuffle test FileCheck patterns for LLVM optimizer improvements: `and`-mask simplifications (e.g. `and i32 %5, -64` → `and i32 %5, 64`) and `add nuw nsw` flag additions, enabled by upstream KnownBits analysis for `mbcnt_lo`/`mbcnt_hi` intrinsics in [686987a540bc](llvm/llvm-project@686987a) ([#183229](llvm/llvm-project#183229)) - Fix GEP element type FileCheck patterns: InstCombine now canonicalizes GEP element types to byte arrays (`float` → `[4 x i8]`, `i64` → `[8 x i8]`) - Fix `mo.while` body type mismatch error message: MLIR's `RegionBranchOpInterface` (`verifyWithRegions`) now detects yield type mismatches before `LoopLikeOpInterface`, emitting a different diagnostic, following upstream [b28ec5ad1808](llvm/llvm-project@b28ec5a) - Regenerate MLIR Python binding stubs MODULAR_ORIG_COMMIT_REV_ID: acd27a457aa9115fd91f73d7ba3338365ece3eb4
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
FuncOp::verify() iterated over all blocks and called getMutableSuccessorOperands() on any RegionBranchTerminatorOpInterface terminator to check return types. This ran during the entrance phase of verification — before child ops had been verified — so a malformed terminator whose getMutableSuccessorOperands() assumed invariants established by its own verify() could crash instead of emitting a clean diagnostic.
Fix by switching to hasRegionVerifier=1: rename verify() → verifyRegions() so the return-type checks run in the exit phase, after all nested ops have already been verified.
To demonstrate the bug and guard against regression, add TestCrashingReturnOp to the test dialect. The op implements RegionBranchTerminatorOpInterface and report_fatal_errors in getMutableSuccessorOperands() when its 'valid' unit-attr is absent, reproducing the class of crash described above. The accompanying lit test confirms a clean diagnostic is emitted rather than a crash.