Skip to content

Commit 2f7e685

Browse files
authored
[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`
1 parent 90c001a commit 2f7e685

File tree

2 files changed

+30
-3
lines changed

2 files changed

+30
-3
lines changed

mlir/lib/IR/Verifier.cpp

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -220,10 +220,15 @@ LogicalResult OperationVerifier::verifyOnExit(Operation &op) {
220220
o.hasTrait<OpTrait::IsIsolatedFromAbove>())
221221
opsWithIsolatedRegions.push_back(&o);
222222
}
223-
if (failed(failableParallelForEach(
224-
op.getContext(), opsWithIsolatedRegions,
225-
[&](Operation *o) { return verifyOpAndDominance(*o); })))
223+
224+
std::atomic<bool> opFailedVerify = false;
225+
parallelForEach(op.getContext(), opsWithIsolatedRegions, [&](Operation *o) {
226+
if (failed(verifyOpAndDominance(*o)))
227+
opFailedVerify.store(true, std::memory_order_relaxed);
228+
});
229+
if (opFailedVerify.load(std::memory_order_relaxed))
226230
return failure();
231+
227232
OperationName opName = op.getName();
228233
std::optional<RegisteredOperationName> registeredInfo =
229234
opName.getRegisteredInfo();

mlir/test/IR/invalid-ops.mlir

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -123,3 +123,25 @@ func.func @invalid_splat(%v : f32) { // expected-note {{prior use here}}
123123

124124
// expected-error@+1 {{number of operands and types do not match: got 0 operands and 1 types}}
125125
test.variadic_args_types_split "hello_world" : i32
126+
127+
// -----
128+
129+
// Test multiple verifier errors in the same split to ensure all are reported.
130+
131+
func.func @verify_fail_1() {
132+
// expected-error@+1 {{'arith.constant' op integer return type must be signless}}
133+
%r = "arith.constant"() {value = -1 : si32} : () -> si32
134+
return
135+
}
136+
137+
func.func @verify_fail_2() {
138+
// expected-error@+1 {{'arith.constant' op value must be an integer, float, or elements attribute}}
139+
%r = "arith.constant"() {value = "hi" : i32} : () -> i32
140+
return
141+
}
142+
143+
func.func @verify_fail_3() {
144+
// expected-error@+1 {{'arith.constant' op integer return type must be signless}}
145+
%r = "arith.constant"() {value = -3 : si32} : () -> si32
146+
return
147+
}

0 commit comments

Comments
 (0)