Skip to content

Commit 0de48de

Browse files
skatrakskatrak
authored
[MLIR][OpenMP] Improve loop wrapper op verifiers (#134833)
This patch revisits op verifiers for `LoopWrapperInterface` operations to improve consistency across operations and to properly cover some previously misreported cases. Checks that should be done for these kinds of operations are documented in the interface description.
1 parent c80080f commit 0de48de

File tree

4 files changed

+87
-24
lines changed

4 files changed

+87
-24
lines changed

‎mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -396,6 +396,7 @@ def WorkshareLoopWrapperOp : OpenMP_Op<"workshare.loop_wrapper", traits = [
396396
];
397397
let assemblyFormat = "$region attr-dict";
398398
let hasVerifier = 1;
399+
let hasRegionVerifier = 1;
399400
}
400401

401402
//===----------------------------------------------------------------------===//

‎mlir/include/mlir/Dialect/OpenMP/OpenMPOpsInterfaces.td

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -222,6 +222,15 @@ def LoopWrapperInterface : OpInterface<"LoopWrapperInterface"> {
222222
single region with a single block in which there's a single operation and a
223223
terminator. That nested operation must be another loop wrapper or an
224224
`omp.loop_nest`.
225+
226+
Operation-specific verifiers should make the following checks in their
227+
verifier, additionally to what the interface itself checks:
228+
- If `getNestedWrapper() != nullptr`, is the type of the nested wrapper
229+
allowed in that context? This check might require looking at the parent as
230+
well.
231+
- If the operation is a `ComposableOpInterface`, check that it is
232+
consistent with the potential existence of a `LoopWrapperInterface` parent
233+
and whether `getNestedWrapper() != nullptr`.
225234
}];
226235

227236
let cppNamespace = "::mlir::omp";
@@ -255,7 +264,7 @@ def LoopWrapperInterface : OpInterface<"LoopWrapperInterface"> {
255264
];
256265

257266
let extraClassDeclaration = [{
258-
/// Interface verifier imlementation.
267+
/// Interface verifier implementation.
259268
llvm::LogicalResult verifyImpl();
260269
}];
261270

‎mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp

Lines changed: 27 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2222,21 +2222,27 @@ LogicalResult ParallelOp::verify() {
22222222
}
22232223

22242224
LogicalResult ParallelOp::verifyRegions() {
2225-
auto distributeChildOps = getOps<DistributeOp>();
2226-
if (!distributeChildOps.empty()) {
2225+
auto distChildOps = getOps<DistributeOp>();
2226+
int numDistChildOps = std::distance(distChildOps.begin(), distChildOps.end());
2227+
if (numDistChildOps > 1)
2228+
return emitError()
2229+
<< "multiple 'omp.distribute' nested inside of 'omp.parallel'";
2230+
2231+
if (numDistChildOps == 1) {
22272232
if (!isComposite())
22282233
return emitError()
22292234
<< "'omp.composite' attribute missing from composite operation";
22302235

22312236
auto *ompDialect = getContext()->getLoadedDialect<OpenMPDialect>();
2232-
Operation &distributeOp = **distributeChildOps.begin();
2237+
Operation &distributeOp = **distChildOps.begin();
22332238
for (Operation &childOp : getOps()) {
22342239
if (&childOp == &distributeOp || ompDialect != childOp.getDialect())
22352240
continue;
22362241

22372242
if (!childOp.hasTrait<OpTrait::IsTerminator>())
22382243
return emitError() << "unexpected OpenMP operation inside of composite "
2239-
"'omp.parallel'";
2244+
"'omp.parallel': "
2245+
<< childOp.getName();
22402246
}
22412247
} else if (isComposite()) {
22422248
return emitError()
@@ -2388,9 +2394,15 @@ void WorkshareOp::build(OpBuilder &builder, OperationState &state,
23882394

23892395
LogicalResult WorkshareLoopWrapperOp::verify() {
23902396
if (!(*this)->getParentOfType<WorkshareOp>())
2391-
return emitError() << "must be nested in an omp.workshare";
2392-
if (getNestedWrapper())
2393-
return emitError() << "cannot be composite";
2397+
return emitOpError() << "must be nested in an omp.workshare";
2398+
return success();
2399+
}
2400+
2401+
LogicalResult WorkshareLoopWrapperOp::verifyRegions() {
2402+
if (isa_and_nonnull<LoopWrapperInterface>((*this)->getParentOp()) ||
2403+
getNestedWrapper())
2404+
return emitOpError() << "expected to be a standalone loop wrapper";
2405+
23942406
return success();
23952407
}
23962408

@@ -2415,7 +2427,7 @@ LogicalResult LoopWrapperInterface::verifyImpl() {
24152427

24162428
Operation &firstOp = *region.op_begin();
24172429
if (!isa<LoopNestOp, LoopWrapperInterface>(firstOp))
2418-
return emitOpError() << "op nested in loop wrapper is not another loop "
2430+
return emitOpError() << "nested in loop wrapper is not another loop "
24192431
"wrapper or `omp.loop_nest`";
24202432

24212433
return success();
@@ -2444,7 +2456,7 @@ LogicalResult LoopOp::verify() {
24442456
LogicalResult LoopOp::verifyRegions() {
24452457
if (llvm::isa_and_nonnull<LoopWrapperInterface>((*this)->getParentOp()) ||
24462458
getNestedWrapper())
2447-
return emitError() << "`omp.loop` expected to be a standalone loop wrapper";
2459+
return emitOpError() << "expected to be a standalone loop wrapper";
24482460

24492461
return success();
24502462
}
@@ -2601,9 +2613,13 @@ LogicalResult DistributeOp::verifyRegions() {
26012613
// Check for the allowed leaf constructs that may appear in a composite
26022614
// construct directly after DISTRIBUTE.
26032615
if (isa<WsloopOp>(nested)) {
2604-
if (!llvm::dyn_cast_if_present<ParallelOp>((*this)->getParentOp()))
2616+
Operation *parentOp = (*this)->getParentOp();
2617+
if (!llvm::dyn_cast_if_present<ParallelOp>(parentOp) ||
2618+
!cast<ComposableOpInterface>(parentOp).isComposite()) {
26052619
return emitError() << "an 'omp.wsloop' nested wrapper is only allowed "
2606-
"when 'omp.parallel' is the direct parent";
2620+
"when a composite 'omp.parallel' is the direct "
2621+
"parent";
2622+
}
26072623
} else if (!isa<SimdOp>(nested))
26082624
return emitError() << "only supported nested wrappers are 'omp.simd' and "
26092625
"'omp.wsloop'";

‎mlir/test/Dialect/OpenMP/invalid.mlir

Lines changed: 49 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -2391,7 +2391,7 @@ func.func @omp_distribute_allocate(%data_var : memref<i32>, %lb : i32, %ub : i32
23912391
// -----
23922392

23932393
func.func @omp_distribute_nested_wrapper(%lb: index, %ub: index, %step: index) -> () {
2394-
// expected-error @below {{an 'omp.wsloop' nested wrapper is only allowed when 'omp.parallel' is the direct parent}}
2394+
// expected-error @below {{an 'omp.wsloop' nested wrapper is only allowed when a composite 'omp.parallel' is the direct parent}}
23952395
omp.distribute {
23962396
"omp.wsloop"() ({
23972397
omp.loop_nest (%iv) : index = (%lb) to (%ub) step (%step) {
@@ -2429,6 +2429,22 @@ func.func @omp_distribute_nested_wrapper3(%lb: index, %ub: index, %step: index)
24292429

24302430
// -----
24312431

2432+
func.func @omp_distribute_nested_wrapper4(%lb: index, %ub: index, %step: index) -> () {
2433+
omp.parallel {
2434+
// expected-error @below {{an 'omp.wsloop' nested wrapper is only allowed when a composite 'omp.parallel' is the direct parent}}
2435+
omp.distribute {
2436+
"omp.wsloop"() ({
2437+
omp.loop_nest (%iv) : index = (%lb) to (%ub) step (%step) {
2438+
"omp.yield"() : () -> ()
2439+
}
2440+
}) {omp.composite} : () -> ()
2441+
} {omp.composite}
2442+
omp.terminator
2443+
}
2444+
}
2445+
2446+
// -----
2447+
24322448
func.func @omp_distribute_order() -> () {
24332449
// expected-error @below {{invalid clause value: 'default'}}
24342450
omp.distribute order(default) {
@@ -2623,15 +2639,13 @@ func.func @masked_arg_count_mismatch(%arg0: i32, %arg1: i32) {
26232639

26242640
// -----
26252641
func.func @omp_parallel_missing_composite(%lb: index, %ub: index, %step: index) -> () {
2626-
// expected-error@+1 {{'omp.composite' attribute missing from composite operation}}
2642+
// expected-error @below {{'omp.composite' attribute missing from composite operation}}
26272643
omp.parallel {
26282644
omp.distribute {
2629-
omp.wsloop {
2630-
omp.loop_nest (%iv) : index = (%lb) to (%ub) step (%step) {
2631-
omp.yield
2632-
}
2633-
} {omp.composite}
2634-
} {omp.composite}
2645+
omp.loop_nest (%iv) : index = (%lb) to (%ub) step (%step) {
2646+
omp.yield
2647+
}
2648+
}
26352649
omp.terminator
26362650
}
26372651
return
@@ -2653,7 +2667,7 @@ func.func @omp_parallel_invalid_composite(%lb: index, %ub: index, %step: index)
26532667

26542668
// -----
26552669
func.func @omp_parallel_invalid_composite2(%lb: index, %ub: index, %step: index) -> () {
2656-
// expected-error @below {{unexpected OpenMP operation inside of composite 'omp.parallel'}}
2670+
// expected-error @below {{unexpected OpenMP operation inside of composite 'omp.parallel': omp.barrier}}
26572671
omp.parallel {
26582672
omp.barrier
26592673
omp.distribute {
@@ -2668,6 +2682,29 @@ func.func @omp_parallel_invalid_composite2(%lb: index, %ub: index, %step: index)
26682682
return
26692683
}
26702684

2685+
// -----
2686+
func.func @omp_parallel_invalid_composite3(%lb: index, %ub: index, %step: index) -> () {
2687+
// expected-error @below {{multiple 'omp.distribute' nested inside of 'omp.parallel'}}
2688+
omp.parallel {
2689+
omp.distribute {
2690+
omp.wsloop {
2691+
omp.loop_nest (%iv) : index = (%lb) to (%ub) step (%step) {
2692+
omp.yield
2693+
}
2694+
} {omp.composite}
2695+
} {omp.composite}
2696+
omp.distribute {
2697+
omp.wsloop {
2698+
omp.loop_nest (%iv) : index = (%lb) to (%ub) step (%step) {
2699+
omp.yield
2700+
}
2701+
} {omp.composite}
2702+
} {omp.composite}
2703+
omp.terminator
2704+
} {omp.composite}
2705+
return
2706+
}
2707+
26712708
// -----
26722709
func.func @omp_wsloop_missing_composite(%lb: index, %ub: index, %step: index) -> () {
26732710
// expected-error @below {{'omp.composite' attribute missing from composite wrapper}}
@@ -2787,7 +2824,7 @@ func.func @omp_taskloop_invalid_composite(%lb: index, %ub: index, %step: index)
27872824

27882825
func.func @omp_loop_invalid_nesting(%lb : index, %ub : index, %step : index) {
27892826

2790-
// expected-error @below {{`omp.loop` expected to be a standalone loop wrapper}}
2827+
// expected-error @below {{'omp.loop' op expected to be a standalone loop wrapper}}
27912828
omp.loop {
27922829
omp.simd {
27932830
omp.loop_nest (%iv) : index = (%lb) to (%ub) step (%step) {
@@ -2804,7 +2841,7 @@ func.func @omp_loop_invalid_nesting(%lb : index, %ub : index, %step : index) {
28042841
func.func @omp_loop_invalid_nesting2(%lb : index, %ub : index, %step : index) {
28052842

28062843
omp.simd {
2807-
// expected-error @below {{`omp.loop` expected to be a standalone loop wrapper}}
2844+
// expected-error @below {{'omp.loop' op expected to be a standalone loop wrapper}}
28082845
omp.loop {
28092846
omp.loop_nest (%iv) : index = (%lb) to (%ub) step (%step) {
28102847
omp.yield
@@ -2831,7 +2868,7 @@ func.func @omp_loop_invalid_binding(%lb : index, %ub : index, %step : index) {
28312868
// -----
28322869
func.func @nested_wrapper(%idx : index) {
28332870
omp.workshare {
2834-
// expected-error @below {{cannot be composite}}
2871+
// expected-error @below {{'omp.workshare.loop_wrapper' op expected to be a standalone loop wrapper}}
28352872
omp.workshare.loop_wrapper {
28362873
omp.simd {
28372874
omp.loop_nest (%iv) : index = (%idx) to (%idx) step (%idx) {

0 commit comments

Comments
 (0)