[Mlir-commits] [mlir] b28ec5a - [mlir][Func] Fix FuncOp verifier ordering via hasRegionVerifier (#184612)

[llvmlistbot at llvm.org]

Author: Mehdi Amini
Date: 2026-03-04T15:11:14Z
New Revision: b28ec5ad18080d40f62de0eb5a07c9ebb7649451

URL: https://github.com/llvm/llvm-project/commit/b28ec5ad18080d40f62de0eb5a07c9ebb7649451
DIFF: https://github.com/llvm/llvm-project/commit/b28ec5ad18080d40f62de0eb5a07c9ebb7649451.diff

LOG: [mlir][Func] Fix FuncOp verifier ordering via hasRegionVerifier (#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.

Added: 
    

Modified: 
    mlir/include/mlir/Dialect/Func/IR/FuncOps.td
    mlir/lib/Dialect/Func/IR/FuncOps.cpp
    mlir/test/Dialect/Func/invalid.mlir
    mlir/test/lib/Dialect/Test/TestOpDefs.cpp
    mlir/test/lib/Dialect/Test/TestOps.td

Removed: 
    


################################################################################
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/invalid.mlir b/mlir/test/Dialect/Func/invalid.mlir
index 2620d284c1d06..3143bda77ebba 100644
--- a/mlir/test/Dialect/Func/invalid.mlir
+++ b/mlir/test/Dialect/Func/invalid.mlir
@@ -213,3 +213,13 @@ func.func @region_branch_term_count_mismatch(%arg: i32) {
   // expected-error @+1 {{'test.loop_block_term' op has 1 operands, but enclosing function (@region_branch_term_count_mismatch) returns 0}}
   test.loop_block_term iter %arg exit %0
 }
+
+// -----
+
+// 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.
+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]>,