[MLIR] Ensure deterministic parallel verification (#134963)
`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`
diff --git a/mlir/lib/IR/Verifier.cpp b/mlir/lib/IR/Verifier.cpp
index f103137..7fe1f54 100644
--- a/mlir/lib/IR/Verifier.cpp
+++ b/mlir/lib/IR/Verifier.cpp
@@ -220,10 +220,15 @@
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();
diff --git a/mlir/test/IR/invalid-ops.mlir b/mlir/test/IR/invalid-ops.mlir
index c7cef97..12a911c 100644
--- a/mlir/test/IR/invalid-ops.mlir
+++ b/mlir/test/IR/invalid-ops.mlir
@@ -123,3 +123,25 @@
// expected-error@+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@+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@+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@+1 {{'arith.constant' op integer return type must be signless}}
+ %r = "arith.constant"() {value = -3 : si32} : () -> si32
+ return
+}
