[Mlir-commits] [mlir] [MLIR] Ensure deterministic parallel verification (PR #134963)

[Nachi G]

https://github.com/nacgarg updated https://github.com/llvm/llvm-project/pull/134963

>From 07590ac77438361198a7736f8d2e825345c74994 Mon Sep 17 00:00:00 2001
From: Nachiketa Gargi <nachi at modular.com>
Date: Wed, 9 Apr 2025 12:04:10 +1000
Subject: [PATCH 1/2] [MLIR] Fix race condition in MLIR verifier

`failableParallelForEach` will non-deterministically early terminate upon failure, leading to inconsistent and potentially missing diagnostics.

This PR uses `parallelForEach` to ensure all operations are verified and all diagnostics are handled, while tracking the failure state separately.

Other potential fixes include:
- Making `failableParallelForEach` have deterministic early-exit behavior (or have an option for it)
  - I didn't want to change more than what was required (and potentially incur perf hits for unrelated code), but if this is a better fix I'm happy to submit a patch.
  - I think all diagnostics that can be detected from verification failures should be reported, so I don't even think this would be correct behavior anyway

- Adding an option for `failableParallelForEach` to still execute on every element on the range while still returning `LogicalResult`
---
 mlir/lib/IR/Verifier.cpp | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/mlir/lib/IR/Verifier.cpp b/mlir/lib/IR/Verifier.cpp
index f10313767b407..7fe1f54b7c3b1 100644
--- a/mlir/lib/IR/Verifier.cpp
+++ b/mlir/lib/IR/Verifier.cpp
@@ -220,10 +220,15 @@ LogicalResult OperationVerifier::verifyOnExit(Operation &op) {
               o.hasTrait<OpTrait::IsIsolatedFromAbove>())
             opsWithIsolatedRegions.push_back(&o);
   }
-  if (failed(failableParallelForEach(
-          op.getContext(), opsWithIsolatedRegions,
-          [&](Operation *o) { return verifyOpAndDominance(*o); })))
+
+  std::atomic<bool> opFailedVerify = false;
+  parallelForEach(op.getContext(), opsWithIsolatedRegions, [&](Operation *o) {
+    if (failed(verifyOpAndDominance(*o)))
+      opFailedVerify.store(true, std::memory_order_relaxed);
+  });
+  if (opFailedVerify.load(std::memory_order_relaxed))
     return failure();
+
   OperationName opName = op.getName();
   std::optional<RegisteredOperationName> registeredInfo =
       opName.getRegisteredInfo();

>From f6edf2588b77d62702b17ef1f99dabbd43930883 Mon Sep 17 00:00:00 2001
From: Nachiketa Gargi <nachi at modular.com>
Date: Wed, 9 Apr 2025 21:55:31 +1000
Subject: [PATCH 2/2] Add test

---
 mlir/test/IR/invalid-ops.mlir | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/mlir/test/IR/invalid-ops.mlir b/mlir/test/IR/invalid-ops.mlir
index c7cef97c246e8..12a911ca8c826 100644
--- a/mlir/test/IR/invalid-ops.mlir
+++ b/mlir/test/IR/invalid-ops.mlir
@@ -123,3 +123,25 @@ func.func @invalid_splat(%v : f32) { // expected-note {{prior use here}}
 
 // expected-error at +1 {{number of operands and types do not match: got 0 operands and 1 types}}
 test.variadic_args_types_split "hello_world" : i32
+
+// -----
+
+// Test multiple verifier errors in the same split to ensure all are reported.
+
+func.func @verify_fail_1() {
+  // expected-error at +1 {{'arith.constant' op integer return type must be signless}}
+  %r = "arith.constant"() {value = -1 : si32} : () -> si32
+  return
+}
+
+func.func @verify_fail_2() {
+  // expected-error at +1 {{'arith.constant' op value must be an integer, float, or elements attribute}}
+  %r = "arith.constant"() {value = "hi" : i32} : () -> i32
+  return
+}
+
+func.func @verify_fail_3() {
+  // expected-error at +1 {{'arith.constant' op integer return type must be signless}}
+  %r = "arith.constant"() {value = -3 : si32} : () -> si32
+  return
+}