[Mlir-commits] [mlir] Fix the order of operation attribute verification (PR #180698)

[llvmlistbot at llvm.org]

https://github.com/ChiaHungDuan created https://github.com/llvm/llvm-project/pull/180698

The verifiers of these attributes are supposed to verify additional constraints which usually require the invariants, nested ops to be verified first. Move it to the end of verification so that we don't operate on malformed operations.

>From e201f9a34a0e4fef48c59aa6c8b98efeb4af70f6 Mon Sep 17 00:00:00 2001
From: Chia-hung Duan <chiahungduan at google.com>
Date: Tue, 10 Feb 2026 07:31:41 +0000
Subject: [PATCH] Fix the order of operation attribute verification

The verifiers of these attributes are supposed to verify additional
constraints which usually require the invariants, nested ops to be
verified first. Move it to the end of verification so that we don't
operate on malformed operations.
---
 .../Dialect/Transform/IR/TransformDialect.cpp | 14 -----
 mlir/lib/IR/Verifier.cpp                      | 16 +++---
 mlir/test/Dialect/GPU/invalid.mlir            |  4 +-
 mlir/test/Dialect/LLVMIR/invalid.mlir         |  2 +-
 mlir/test/Dialect/Transform/ops-invalid.mlir  | 51 -------------------
 5 files changed, 11 insertions(+), 76 deletions(-)

diff --git a/mlir/lib/Dialect/Transform/IR/TransformDialect.cpp b/mlir/lib/Dialect/Transform/IR/TransformDialect.cpp
index 45cef9c162c70..6aa13d8661e8e 100644
--- a/mlir/lib/Dialect/Transform/IR/TransformDialect.cpp
+++ b/mlir/lib/Dialect/Transform/IR/TransformDialect.cpp
@@ -141,20 +141,6 @@ LogicalResult transform::TransformDialect::verifyOperationAttribute(
                                         "operations with symbol tables";
     }
 
-    // Pre-verify calls and callables because call graph construction below
-    // assumes they are valid, but this verifier runs before verifying the
-    // nested operations.
-    WalkResult walkResult = op->walk([](Operation *nested) {
-      if (!isa<CallableOpInterface, CallOpInterface>(nested))
-        return WalkResult::advance();
-
-      if (failed(verify(nested, /*verifyRecursively=*/false)))
-        return WalkResult::interrupt();
-      return WalkResult::advance();
-    });
-    if (walkResult.wasInterrupted())
-      return failure();
-
     const mlir::CallGraph callgraph(op);
     for (auto scc = llvm::scc_begin(&callgraph); !scc.isAtEnd(); ++scc) {
       if (!scc.hasCycle())
diff --git a/mlir/lib/IR/Verifier.cpp b/mlir/lib/IR/Verifier.cpp
index e19537a901d18..754f82b156f1a 100644
--- a/mlir/lib/IR/Verifier.cpp
+++ b/mlir/lib/IR/Verifier.cpp
@@ -160,14 +160,6 @@ LogicalResult OperationVerifier::verifyOnEntrance(Operation &op) {
     if (!operand)
       return op.emitError("null operand found");
 
-  /// Verify that all of the attributes are okay.
-  for (auto attr : op.getDiscardableAttrDictionary()) {
-    // Check for any optional dialect specific attributes.
-    if (auto *dialect = attr.getNameDialect())
-      if (failed(dialect->verifyOperationAttribute(&op, attr)))
-        return failure();
-  }
-
   // If we can get operation info for this, check the custom hook.
   OperationName opName = op.getName();
   std::optional<RegisteredOperationName> registeredInfo =
@@ -236,6 +228,14 @@ LogicalResult OperationVerifier::verifyOnExit(Operation &op) {
   if (registeredInfo && failed(registeredInfo->verifyRegionInvariants(&op)))
     return failure();
 
+  /// Verify that all of the attributes are okay.
+  for (auto attr : op.getDiscardableAttrDictionary()) {
+    // Check for any optional dialect specific attributes.
+    if (auto *dialect = attr.getNameDialect())
+      if (failed(dialect->verifyOperationAttribute(&op, attr)))
+        return failure();
+  }
+
   // If this is a registered operation, there is nothing left to do.
   if (registeredInfo)
     return success();
diff --git a/mlir/test/Dialect/GPU/invalid.mlir b/mlir/test/Dialect/GPU/invalid.mlir
index 7c678e4f34d3d..c1f98e6f96937 100644
--- a/mlir/test/Dialect/GPU/invalid.mlir
+++ b/mlir/test/Dialect/GPU/invalid.mlir
@@ -183,8 +183,8 @@ module attributes {gpu.container_module} {
 
 module attributes {gpu.container_module} {
   module @kernels {
-    gpu.func @kernel_1(%arg1 : !llvm.ptr) kernel {
-      gpu.return
+    func.func @kernel_1(%arg1 : !llvm.ptr) {
+      return
     }
   }
 
diff --git a/mlir/test/Dialect/LLVMIR/invalid.mlir b/mlir/test/Dialect/LLVMIR/invalid.mlir
index 49b6342aea538..cdbc94f1998d6 100644
--- a/mlir/test/Dialect/LLVMIR/invalid.mlir
+++ b/mlir/test/Dialect/LLVMIR/invalid.mlir
@@ -993,7 +993,7 @@ func.func @invalid_ordering_in_fence() {
 
 // expected-error @+1 {{invalid data layout descriptor}}
 module attributes {llvm.data_layout = "#vjkr32"} {
-  func.func @invalid_data_layout()
+  func.func private @invalid_data_layout()
 }
 
 // -----
diff --git a/mlir/test/Dialect/Transform/ops-invalid.mlir b/mlir/test/Dialect/Transform/ops-invalid.mlir
index 6d4e00822f419..599fab902f61b 100644
--- a/mlir/test/Dialect/Transform/ops-invalid.mlir
+++ b/mlir/test/Dialect/Transform/ops-invalid.mlir
@@ -921,54 +921,3 @@ module attributes { transform.with_named_sequence } {
     transform.yield
   }
 }
-
-// -----
-
-module attributes { transform.with_named_sequence } {
-  transform.named_sequence @__transform_main(%arg0: !transform.any_op) -> () {
-    // Intentionally malformed func with no region. This shouldn't crash the
-    // verifier of `with_named_sequence` that runs before we get to the
-    // function.
-    // expected-error @below {{requires one region}}
-    "func.func"() : () -> ()
-    transform.yield
-  }
-}
-
-// -----
-
-module attributes { transform.with_named_sequence } {
-  transform.named_sequence @__transform_main(%arg0: !transform.any_op) -> () {
-    // Intentionally malformed call with a region. This shouldn't crash the
-    // verifier of `with_named_sequence` that runs before we get to the call.
-    // expected-error @below {{requires zero regions}}
-    "func.call"() <{
-      function_type = () -> (),
-      sym_name = "lambda_function"
-    }> ({
-    ^bb0:
-      "func.return"() : () -> ()
-    }) : () -> ()
-    transform.yield
-  }
-}
-
-// -----
-
-module attributes { transform.with_named_sequence } {
-  // Intentionally malformed sequence where the verifier should not crash.
-  // expected-error @below {{ op expects argument attribute array to have the same number of elements as the number of function arguments, got 1, but expected 3}}
-  "transform.named_sequence"() <{
-    arg_attrs = [{transform.readonly}],
-    function_type = (i1, tensor<f32>, tensor<f32>) -> (),
-    sym_name = "print_message"
-  }> ({}) : () -> ()
-  "transform.named_sequence"() <{
-    function_type = (!transform.any_op) -> (),
-    sym_name = "reference_other_module"
-  }> ({
-  ^bb0(%arg0: !transform.any_op):
-    "transform.include"(%arg0) <{target = @print_message}> : (!transform.any_op) -> ()
-    "transform.yield"() : () -> ()
-  }) : () -> ()
-}