[MLIR][OpenMP] Move loop wrapper verification to the interface (NFC) #110505
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This patch moves verification code for the `LoopWrapperInterface` to the interface itself, checking it automatically for each operation that has that interface.
skatrak
requested review from
ergawy,
Meinersbur,
jsjodin,
tblah,
kiranchandramohan,
mjklemm,
raghavendhra,
agozillon,
DominikAdamski,
TIFitis,
kparzysz,
anchuraj and
bhandarkar-pranav
|
@llvm/pr-subscribers-flang-openmp @llvm/pr-subscribers-mlir-openmp Author: Sergio Afonso (skatrak) ChangesThis patch moves verification code for the Full diff: https://github.com/llvm/llvm-project/pull/110505.diff 3 Files Affected:
diff --git a/mlir/include/mlir/Dialect/OpenMP/OpenMPOpsInterfaces.td b/mlir/include/mlir/Dialect/OpenMP/OpenMPOpsInterfaces.td
index 0078e22b1c89a6..ea1e3ebecef7b4 100644
--- a/mlir/include/mlir/Dialect/OpenMP/OpenMPOpsInterfaces.td
+++ b/mlir/include/mlir/Dialect/OpenMP/OpenMPOpsInterfaces.td
@@ -106,6 +106,15 @@ def LoopWrapperInterface : OpInterface<"LoopWrapperInterface"> {
}]
>
];
+
+ let extraClassDeclaration = [{
+ /// Interface verifier imlementation.
+ llvm::LogicalResult verifyImpl();
+ }];
+
+ let verify = [{
+ return ::llvm::cast<::mlir::omp::LoopWrapperInterface>($_op).verifyImpl();
+ }];
}
def ComposableOpInterface : OpInterface<"ComposableOpInterface"> {
diff --git a/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp b/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
index 90bf5df67b03ba..59e71ecc6ec5df 100644
--- a/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
+++ b/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
@@ -1682,6 +1682,37 @@ LogicalResult SingleOp::verify() {
getCopyprivateSyms());
}
+//===----------------------------------------------------------------------===//
+// LoopWrapperInterface
+//===----------------------------------------------------------------------===//
+
+LogicalResult LoopWrapperInterface::verifyImpl() {
+ Operation *op = this->getOperation();
+ if (op->getNumRegions() != 1)
+ return emitOpError() << "loop wrapper contains multiple regions";
+
+ Region ®ion = op->getRegion(0);
+ if (!region.hasOneBlock())
+ return emitOpError() << "loop wrapper contains multiple blocks";
+
+ if (::llvm::range_size(region.getOps()) != 2)
+ return emitOpError()
+ << "loop wrapper does not contain exactly two nested ops";
+
+ Operation &firstOp = *region.op_begin();
+ Operation &secondOp = *(std::next(region.op_begin()));
+
+ if (!secondOp.hasTrait<OpTrait::IsTerminator>())
+ return emitOpError()
+ << "second nested op in loop wrapper is not a terminator";
+
+ if (!::llvm::isa<LoopNestOp, LoopWrapperInterface>(firstOp))
+ return emitOpError() << "first nested op in loop wrapper is not "
+ "another loop wrapper or `omp.loop_nest`";
+
+ return success();
+}
+
//===----------------------------------------------------------------------===//
// WsloopOp
//===----------------------------------------------------------------------===//
@@ -1714,32 +1745,6 @@ void printWsloop(OpAsmPrinter &p, Operation *op, Region ®ion,
p.printRegion(region, /*printEntryBlockArgs=*/false);
}
-static LogicalResult verifyLoopWrapperInterface(Operation *op) {
- if (op->getNumRegions() != 1)
- return op->emitOpError() << "loop wrapper contains multiple regions";
-
- Region ®ion = op->getRegion(0);
- if (!region.hasOneBlock())
- return op->emitOpError() << "loop wrapper contains multiple blocks";
-
- if (::llvm::range_size(region.getOps()) != 2)
- return op->emitOpError()
- << "loop wrapper does not contain exactly two nested ops";
-
- Operation &firstOp = *region.op_begin();
- Operation &secondOp = *(std::next(region.op_begin()));
-
- if (!secondOp.hasTrait<OpTrait::IsTerminator>())
- return op->emitOpError()
- << "second nested op in loop wrapper is not a terminator";
-
- if (!::llvm::isa<LoopNestOp, LoopWrapperInterface>(firstOp))
- return op->emitOpError() << "first nested op in loop wrapper is not "
- "another loop wrapper or `omp.loop_nest`";
-
- return success();
-}
-
void WsloopOp::build(OpBuilder &builder, OperationState &state,
ArrayRef<NamedAttribute> attributes) {
build(builder, state, /*allocate_vars=*/{}, /*allocator_vars=*/{},
@@ -1770,9 +1775,6 @@ void WsloopOp::build(OpBuilder &builder, OperationState &state,
}
LogicalResult WsloopOp::verify() {
- if (verifyLoopWrapperInterface(*this).failed())
- return failure();
-
bool isCompositeChildLeaf =
llvm::dyn_cast_if_present<LoopWrapperInterface>((*this)->getParentOp());
@@ -1829,9 +1831,6 @@ LogicalResult SimdOp::verify() {
if (verifyNontemporalClause(*this, getNontemporalVars()).failed())
return failure();
- if (verifyLoopWrapperInterface(*this).failed())
- return failure();
-
if (getNestedWrapper())
return emitOpError() << "must wrap an 'omp.loop_nest' directly";
@@ -1871,9 +1870,6 @@ LogicalResult DistributeOp::verify() {
return emitError(
"expected equal sizes for allocate and allocator variables");
- if (verifyLoopWrapperInterface(*this).failed())
- return failure();
-
if (LoopWrapperInterface nested = getNestedWrapper()) {
if (!isComposite())
return emitError()
@@ -2079,9 +2075,6 @@ LogicalResult TaskloopOp::verify() {
"may not appear on the same taskloop directive");
}
- if (verifyLoopWrapperInterface(*this).failed())
- return failure();
-
if (LoopWrapperInterface nested = getNestedWrapper()) {
if (!isComposite())
return emitError()
diff --git a/mlir/test/Dialect/OpenMP/invalid.mlir b/mlir/test/Dialect/OpenMP/invalid.mlir
index d8745f1015af83..35a8883e3a317e 100644
--- a/mlir/test/Dialect/OpenMP/invalid.mlir
+++ b/mlir/test/Dialect/OpenMP/invalid.mlir
@@ -355,6 +355,7 @@ func.func @omp_simd_aligned_mismatch(%arg0 : index, %arg1 : index,
omp.loop_nest (%iv) : index = (%arg0) to (%arg1) step (%arg2) {
omp.yield
}
+ omp.terminator
}) {alignments = [128],
operandSegmentSizes = array<i32: 2, 0, 0, 0, 0, 0, 0>} : (memref<i32>, memref<i32>) -> ()
return
@@ -370,6 +371,7 @@ func.func @omp_simd_aligned_negative(%arg0 : index, %arg1 : index,
omp.loop_nest (%iv) : index = (%arg0) to (%arg1) step (%arg2) {
omp.yield
}
+ omp.terminator
}) {alignments = [-1, 128], operandSegmentSizes = array<i32: 2, 0, 0, 0, 0, 0, 0>} : (memref<i32>, memref<i32>) -> ()
return
}
@@ -384,6 +386,7 @@ func.func @omp_simd_unexpected_alignment(%arg0 : index, %arg1 : index,
omp.loop_nest (%iv) : index = (%arg0) to (%arg1) step (%arg2) {
omp.yield
}
+ omp.terminator
}) {alignments = [1, 128]} : () -> ()
return
}
@@ -398,6 +401,7 @@ func.func @omp_simd_aligned_float(%arg0 : index, %arg1 : index,
omp.loop_nest (%iv) : index = (%arg0) to (%arg1) step (%arg2) {
omp.yield
}
+ omp.terminator
}) {alignments = [1.5, 128], operandSegmentSizes = array<i32: 2, 0, 0, 0, 0, 0, 0>} : (memref<i32>, memref<i32>) -> ()
return
}
@@ -412,6 +416,7 @@ func.func @omp_simd_aligned_the_same_var(%arg0 : index, %arg1 : index,
omp.loop_nest (%iv) : index = (%arg0) to (%arg1) step (%arg2) {
omp.yield
}
+ omp.terminator
}) {alignments = [1, 128], operandSegmentSizes = array<i32: 2, 0, 0, 0, 0, 0, 0>} : (memref<i32>, memref<i32>) -> ()
return
}
@@ -426,6 +431,7 @@ func.func @omp_simd_nontemporal_the_same_var(%arg0 : index, %arg1 : index,
omp.loop_nest (%iv) : index = (%arg0) to (%arg1) step (%arg2) {
omp.yield
}
+ omp.terminator
}) {operandSegmentSizes = array<i32: 0, 0, 0, 0, 2, 0, 0>} : (memref<i32>, memref<i32>) -> ()
return
}
@@ -438,6 +444,7 @@ func.func @omp_simd_order_value(%lb : index, %ub : index, %step : index) {
omp.loop_nest (%iv) : index = (%arg0) to (%arg1) step (%arg2) {
omp.yield
}
+ omp.terminator
}
return
}
@@ -450,6 +457,7 @@ func.func @omp_simd_reproducible_order(%lb : index, %ub : index, %step : index)
omp.loop_nest (%iv) : index = (%arg0) to (%arg1) step (%arg2) {
omp.yield
}
+ omp.terminator
}
return
}
@@ -460,6 +468,7 @@ func.func @omp_simd_unconstrained_order(%lb : index, %ub : index, %step : index)
omp.loop_nest (%iv) : index = (%arg0) to (%arg1) step (%arg2) {
omp.yield
}
+ omp.terminator
}
return
}
@@ -470,6 +479,7 @@ func.func @omp_simd_pretty_simdlen(%lb : index, %ub : index, %step : index) -> (
omp.loop_nest (%iv) : index = (%lb) to (%ub) step (%step) {
omp.yield
}
+ omp.terminator
}
return
}
@@ -482,6 +492,7 @@ func.func @omp_simd_pretty_safelen(%lb : index, %ub : index, %step : index) -> (
omp.loop_nest (%iv) : index = (%lb) to (%ub) step (%step) {
omp.yield
}
+ omp.terminator
}
return
}
@@ -494,6 +505,7 @@ func.func @omp_simd_pretty_simdlen_safelen(%lb : index, %ub : index, %step : ind
omp.loop_nest (%iv) : index = (%lb) to (%ub) step (%step) {
omp.yield
}
+ omp.terminator
}
return
}
@@ -1838,6 +1850,7 @@ func.func @taskloop(%lb: i32, %ub: i32, %step: i32) {
omp.loop_nest (%i, %j) : i32 = (%lb, %ub) to (%ub, %lb) step (%step, %step) {
omp.yield
}
+ omp.terminator
}) {operandSegmentSizes = array<i32: 1, 0, 0, 0, 0, 0, 0, 0, 0, 0>} : (memref<i32>) -> ()
return
}
@@ -1852,6 +1865,7 @@ func.func @taskloop(%lb: i32, %ub: i32, %step: i32) {
omp.loop_nest (%i, %j) : i32 = (%lb, %ub) to (%ub, %lb) step (%step, %step) {
omp.yield
}
+ omp.terminator
}) {operandSegmentSizes = array<i32: 0, 0, 0, 0, 0, 0, 0, 0, 0, 2>, reduction_syms = [@add_f32]} : (!llvm.ptr, !llvm.ptr) -> ()
return
}
@@ -1865,6 +1879,7 @@ func.func @taskloop(%lb: i32, %ub: i32, %step: i32) {
omp.loop_nest (%i, %j) : i32 = (%lb, %ub) to (%ub, %lb) step (%step, %step) {
omp.yield
}
+ omp.terminator
}) {operandSegmentSizes = array<i32: 0, 0, 0, 0, 0, 0, 0, 0, 0, 1>, reduction_syms = [@add_f32, @add_f32]} : (!llvm.ptr) -> ()
return
}
@@ -1879,6 +1894,7 @@ func.func @taskloop(%lb: i32, %ub: i32, %step: i32) {
omp.loop_nest (%i, %j) : i32 = (%lb, %ub) to (%ub, %lb) step (%step, %step) {
omp.yield
}
+ omp.terminator
}) {in_reduction_syms = [@add_f32], operandSegmentSizes = array<i32: 0, 0, 0, 0, 0, 2, 0, 0, 0, 0>} : (!llvm.ptr, !llvm.ptr) -> ()
return
}
@@ -1892,6 +1908,7 @@ func.func @taskloop(%lb: i32, %ub: i32, %step: i32) {
omp.loop_nest (%i, %j) : i32 = (%lb, %ub) to (%ub, %lb) step (%step, %step) {
omp.yield
}
+ omp.terminator
}) {in_reduction_syms = [@add_f32, @add_f32], operandSegmentSizes = array<i32: 0, 0, 0, 0, 0, 1, 0, 0, 0, 0>} : (!llvm.ptr) -> ()
return
}
@@ -1918,6 +1935,7 @@ func.func @taskloop(%lb: i32, %ub: i32, %step: i32) {
omp.loop_nest (%i, %j) : i32 = (%lb, %ub) to (%ub, %lb) step (%step, %step) {
omp.yield
}
+ omp.terminator
}
return
}
@@ -1943,6 +1961,7 @@ func.func @taskloop(%lb: i32, %ub: i32, %step: i32) {
omp.loop_nest (%i, %j) : i32 = (%lb, %ub) to (%ub, %lb) step (%step, %step) {
omp.yield
}
+ omp.terminator
}
return
}
@@ -1956,6 +1975,7 @@ func.func @taskloop(%lb: i32, %ub: i32, %step: i32) {
omp.loop_nest (%i, %j) : i32 = (%lb, %ub) to (%ub, %lb) step (%step, %step) {
omp.yield
}
+ omp.terminator
}
return
}
@@ -2153,20 +2173,26 @@ func.func @omp_target_depend(%data_var: memref<i32>) {
// -----
-func.func @omp_distribute_schedule(%chunk_size : i32) -> () {
+func.func @omp_distribute_schedule(%chunk_size : i32, %lb : i32, %ub : i32, %step : i32) -> () {
// expected-error @below {{op chunk size set without dist_schedule_static being present}}
"omp.distribute"(%chunk_size) <{operandSegmentSizes = array<i32: 0, 0, 1, 0>}> ({
- "omp.terminator"() : () -> ()
- }) : (i32) -> ()
+ omp.loop_nest (%iv) : i32 = (%lb) to (%ub) step (%step) {
+ "omp.yield"() : () -> ()
+ }
+ "omp.terminator"() : () -> ()
+ }) : (i32) -> ()
}
// -----
-func.func @omp_distribute_allocate(%data_var : memref<i32>) -> () {
+func.func @omp_distribute_allocate(%data_var : memref<i32>, %lb : i32, %ub : i32, %step : i32) -> () {
// expected-error @below {{expected equal sizes for allocate and allocator variables}}
"omp.distribute"(%data_var) <{operandSegmentSizes = array<i32: 1, 0, 0, 0>}> ({
- "omp.terminator"() : () -> ()
- }) : (memref<i32>) -> ()
+ omp.loop_nest (%iv) : i32 = (%lb) to (%ub) step (%step) {
+ "omp.yield"() : () -> ()
+ }
+ "omp.terminator"() : () -> ()
+ }) : (memref<i32>) -> ()
}
// -----
@@ -2174,10 +2200,10 @@ func.func @omp_distribute_allocate(%data_var : memref<i32>) -> () {
func.func @omp_distribute_wrapper(%lb: index, %ub: index, %step: index) -> () {
// expected-error @below {{op second nested op in loop wrapper is not a terminator}}
omp.distribute {
- omp.loop_nest (%iv) : index = (%lb) to (%ub) step (%step) {
- "omp.yield"() : () -> ()
- }
- %0 = arith.constant 0 : i32
+ omp.loop_nest (%iv) : index = (%lb) to (%ub) step (%step) {
+ "omp.yield"() : () -> ()
+ }
+ %0 = arith.constant 0 : i32
}
}
|
|
If the second operation is always |
My first approach was indeed making these operations I guess now that things are a bit more fleshed out I could look again into this. Maybe after landing the PR stack to unify the handling of block argument-defining ops, because I seem to remember there was some interaction between custom parsing of the region and the implicit terminators. Let me know if that works for you. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch moves verification code for the
LoopWrapperInterfaceto the interface itself, checking it automatically for each operation that has that interface.