[MLIR][OpenMP] Split region-associated op verification #112355
Merged
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
skatrak
requested review from
ergawy,
Meinersbur,
jsjodin,
tblah,
kiranchandramohan,
abidh,
mjklemm,
raghavendhra,
agozillon,
DominikAdamski,
TIFitis,
kparzysz,
anchuraj and
bhandarkar-pranav
|
@llvm/pr-subscribers-mlir-openmp @llvm/pr-subscribers-mlir Author: Sergio Afonso (skatrak) ChangesThis patch moves the part of operation verifiers dependent on the contents of their regions to the corresponding The Full diff: https://github.com/llvm/llvm-project/pull/112355.diff 4 Files Affected:
diff --git a/mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td b/mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
index 11649ef2d03370..45313200d4f0b9 100644
--- a/mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
+++ b/mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
@@ -137,7 +137,7 @@ def PrivateClauseOp : OpenMP_Op<"private", [IsolatedFromAbove, RecipeInterface]>
}
}];
- let hasVerifier = 1;
+ let hasRegionVerifier = 1;
}
//===----------------------------------------------------------------------===//
@@ -175,6 +175,7 @@ def ParallelOp : OpenMP_Op<"parallel", traits = [
}];
let hasVerifier = 1;
+ let hasRegionVerifier = 1;
}
def TerminatorOp : OpenMP_Op<"terminator", [Terminator, Pure]> {
@@ -426,6 +427,7 @@ def WsloopOp : OpenMP_Op<"wsloop", traits = [
}];
let hasVerifier = 1;
+ let hasRegionVerifier = 1;
}
//===----------------------------------------------------------------------===//
@@ -479,6 +481,7 @@ def SimdOp : OpenMP_Op<"simd", traits = [
}];
let hasVerifier = 1;
+ let hasRegionVerifier = 1;
}
@@ -556,6 +559,7 @@ def DistributeOp : OpenMP_Op<"distribute", traits = [
}];
let hasVerifier = 1;
+ let hasRegionVerifier = 1;
}
//===----------------------------------------------------------------------===//
@@ -693,6 +697,7 @@ def TaskloopOp : OpenMP_Op<"taskloop", traits = [
}] # clausesExtraClassDeclaration;
let hasVerifier = 1;
+ let hasRegionVerifier = 1;
}
def TaskgroupOp : OpenMP_Op<"taskgroup", traits = [
diff --git a/mlir/include/mlir/Dialect/OpenMP/OpenMPOpsInterfaces.td b/mlir/include/mlir/Dialect/OpenMP/OpenMPOpsInterfaces.td
index 22521b08637cf8..8b72689dc3fd87 100644
--- a/mlir/include/mlir/Dialect/OpenMP/OpenMPOpsInterfaces.td
+++ b/mlir/include/mlir/Dialect/OpenMP/OpenMPOpsInterfaces.td
@@ -258,6 +258,7 @@ def LoopWrapperInterface : OpInterface<"LoopWrapperInterface"> {
let verify = [{
return ::llvm::cast<::mlir::omp::LoopWrapperInterface>($_op).verifyImpl();
}];
+ let verifyWithRegions = 1;
}
def ComposableOpInterface : OpInterface<"ComposableOpInterface"> {
diff --git a/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp b/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
index c6c6edb8f999fc..c03683df63cec8 100644
--- a/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
+++ b/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
@@ -1760,6 +1760,18 @@ static LogicalResult verifyPrivateVarList(OpType &op) {
}
LogicalResult ParallelOp::verify() {
+ if (getAllocateVars().size() != getAllocatorVars().size())
+ return emitError(
+ "expected equal sizes for allocate and allocator variables");
+
+ if (failed(verifyPrivateVarList(*this)))
+ return failure();
+
+ return verifyReductionVarList(*this, getReductionSyms(), getReductionVars(),
+ getReductionByref());
+}
+
+LogicalResult ParallelOp::verifyRegions() {
auto distributeChildOps = getOps<DistributeOp>();
if (!distributeChildOps.empty()) {
if (!isComposite())
@@ -1780,16 +1792,7 @@ LogicalResult ParallelOp::verify() {
return emitError()
<< "'omp.composite' attribute present in non-composite operation";
}
-
- if (getAllocateVars().size() != getAllocatorVars().size())
- return emitError(
- "expected equal sizes for allocate and allocator variables");
-
- if (failed(verifyPrivateVarList(*this)))
- return failure();
-
- return verifyReductionVarList(*this, getReductionSyms(), getReductionVars(),
- getReductionByref());
+ return success();
}
//===----------------------------------------------------------------------===//
@@ -1979,6 +1982,11 @@ void WsloopOp::build(OpBuilder &builder, OperationState &state,
}
LogicalResult WsloopOp::verify() {
+ return verifyReductionVarList(*this, getReductionSyms(), getReductionVars(),
+ getReductionByref());
+}
+
+LogicalResult WsloopOp::verifyRegions() {
bool isCompositeChildLeaf =
llvm::dyn_cast_if_present<LoopWrapperInterface>((*this)->getParentOp());
@@ -2000,8 +2008,7 @@ LogicalResult WsloopOp::verify() {
<< "'omp.composite' attribute missing from composite wrapper";
}
- return verifyReductionVarList(*this, getReductionSyms(), getReductionVars(),
- getReductionByref());
+ return success();
}
//===----------------------------------------------------------------------===//
@@ -2035,9 +2042,6 @@ LogicalResult SimdOp::verify() {
if (verifyNontemporalClause(*this, getNontemporalVars()).failed())
return failure();
- if (getNestedWrapper())
- return emitOpError() << "must wrap an 'omp.loop_nest' directly";
-
bool isCompositeChildLeaf =
llvm::dyn_cast_if_present<LoopWrapperInterface>((*this)->getParentOp());
@@ -2052,6 +2056,13 @@ LogicalResult SimdOp::verify() {
return success();
}
+LogicalResult SimdOp::verifyRegions() {
+ if (getNestedWrapper())
+ return emitOpError() << "must wrap an 'omp.loop_nest' directly";
+
+ return success();
+}
+
//===----------------------------------------------------------------------===//
// Distribute construct [2.9.4.1]
//===----------------------------------------------------------------------===//
@@ -2074,6 +2085,10 @@ LogicalResult DistributeOp::verify() {
return emitError(
"expected equal sizes for allocate and allocator variables");
+ return success();
+}
+
+LogicalResult DistributeOp::verifyRegions() {
if (LoopWrapperInterface nested = getNestedWrapper()) {
if (!isComposite())
return emitError()
@@ -2279,6 +2294,10 @@ LogicalResult TaskloopOp::verify() {
"may not appear on the same taskloop directive");
}
+ return success();
+}
+
+LogicalResult TaskloopOp::verifyRegions() {
if (LoopWrapperInterface nested = getNestedWrapper()) {
if (!isComposite())
return emitError()
@@ -2723,7 +2742,7 @@ void PrivateClauseOp::build(OpBuilder &odsBuilder, OperationState &odsState,
DataSharingClauseType::Private));
}
-LogicalResult PrivateClauseOp::verify() {
+LogicalResult PrivateClauseOp::verifyRegions() {
Type symType = getType();
auto verifyTerminator = [&](Operation *terminator,
diff --git a/mlir/test/Dialect/OpenMP/invalid.mlir b/mlir/test/Dialect/OpenMP/invalid.mlir
index f7a87713aca351..fd89ec31c64a60 100644
--- a/mlir/test/Dialect/OpenMP/invalid.mlir
+++ b/mlir/test/Dialect/OpenMP/invalid.mlir
@@ -136,9 +136,11 @@ func.func @invalid_nested_wrapper(%lb : index, %ub : index, %step : index) {
// expected-error @below {{only supported nested wrapper is 'omp.simd'}}
omp.wsloop {
omp.distribute {
- omp.loop_nest (%iv) : index = (%lb) to (%ub) step (%step) {
- omp.yield
- }
+ omp.simd {
+ omp.loop_nest (%iv) : index = (%lb) to (%ub) step (%step) {
+ omp.yield
+ }
+ } {omp.composite}
} {omp.composite}
} {omp.composite}
}
@@ -1975,7 +1977,7 @@ func.func @taskloop(%lb: i32, %ub: i32, %step: i32) {
omp.loop_nest (%iv) : i32 = (%lb) to (%ub) step (%step) {
omp.yield
}
- } {omp.composite}
+ }
} {omp.composite}
return
}
@@ -2188,7 +2190,7 @@ func.func @omp_distribute_nested_wrapper2(%lb: index, %ub: index, %step: index)
omp.loop_nest (%iv) : index = (%lb) to (%ub) step (%step) {
"omp.yield"() : () -> ()
}
- }) {omp.composite} : () -> ()
+ }) : () -> ()
} {omp.composite}
}
|
|
@llvm/pr-subscribers-flang-openmp Author: Sergio Afonso (skatrak) ChangesThis patch moves the part of operation verifiers dependent on the contents of their regions to the corresponding The Full diff: https://github.com/llvm/llvm-project/pull/112355.diff 4 Files Affected:
diff --git a/mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td b/mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
index 11649ef2d03370..45313200d4f0b9 100644
--- a/mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
+++ b/mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
@@ -137,7 +137,7 @@ def PrivateClauseOp : OpenMP_Op<"private", [IsolatedFromAbove, RecipeInterface]>
}
}];
- let hasVerifier = 1;
+ let hasRegionVerifier = 1;
}
//===----------------------------------------------------------------------===//
@@ -175,6 +175,7 @@ def ParallelOp : OpenMP_Op<"parallel", traits = [
}];
let hasVerifier = 1;
+ let hasRegionVerifier = 1;
}
def TerminatorOp : OpenMP_Op<"terminator", [Terminator, Pure]> {
@@ -426,6 +427,7 @@ def WsloopOp : OpenMP_Op<"wsloop", traits = [
}];
let hasVerifier = 1;
+ let hasRegionVerifier = 1;
}
//===----------------------------------------------------------------------===//
@@ -479,6 +481,7 @@ def SimdOp : OpenMP_Op<"simd", traits = [
}];
let hasVerifier = 1;
+ let hasRegionVerifier = 1;
}
@@ -556,6 +559,7 @@ def DistributeOp : OpenMP_Op<"distribute", traits = [
}];
let hasVerifier = 1;
+ let hasRegionVerifier = 1;
}
//===----------------------------------------------------------------------===//
@@ -693,6 +697,7 @@ def TaskloopOp : OpenMP_Op<"taskloop", traits = [
}] # clausesExtraClassDeclaration;
let hasVerifier = 1;
+ let hasRegionVerifier = 1;
}
def TaskgroupOp : OpenMP_Op<"taskgroup", traits = [
diff --git a/mlir/include/mlir/Dialect/OpenMP/OpenMPOpsInterfaces.td b/mlir/include/mlir/Dialect/OpenMP/OpenMPOpsInterfaces.td
index 22521b08637cf8..8b72689dc3fd87 100644
--- a/mlir/include/mlir/Dialect/OpenMP/OpenMPOpsInterfaces.td
+++ b/mlir/include/mlir/Dialect/OpenMP/OpenMPOpsInterfaces.td
@@ -258,6 +258,7 @@ def LoopWrapperInterface : OpInterface<"LoopWrapperInterface"> {
let verify = [{
return ::llvm::cast<::mlir::omp::LoopWrapperInterface>($_op).verifyImpl();
}];
+ let verifyWithRegions = 1;
}
def ComposableOpInterface : OpInterface<"ComposableOpInterface"> {
diff --git a/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp b/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
index c6c6edb8f999fc..c03683df63cec8 100644
--- a/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
+++ b/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
@@ -1760,6 +1760,18 @@ static LogicalResult verifyPrivateVarList(OpType &op) {
}
LogicalResult ParallelOp::verify() {
+ if (getAllocateVars().size() != getAllocatorVars().size())
+ return emitError(
+ "expected equal sizes for allocate and allocator variables");
+
+ if (failed(verifyPrivateVarList(*this)))
+ return failure();
+
+ return verifyReductionVarList(*this, getReductionSyms(), getReductionVars(),
+ getReductionByref());
+}
+
+LogicalResult ParallelOp::verifyRegions() {
auto distributeChildOps = getOps<DistributeOp>();
if (!distributeChildOps.empty()) {
if (!isComposite())
@@ -1780,16 +1792,7 @@ LogicalResult ParallelOp::verify() {
return emitError()
<< "'omp.composite' attribute present in non-composite operation";
}
-
- if (getAllocateVars().size() != getAllocatorVars().size())
- return emitError(
- "expected equal sizes for allocate and allocator variables");
-
- if (failed(verifyPrivateVarList(*this)))
- return failure();
-
- return verifyReductionVarList(*this, getReductionSyms(), getReductionVars(),
- getReductionByref());
+ return success();
}
//===----------------------------------------------------------------------===//
@@ -1979,6 +1982,11 @@ void WsloopOp::build(OpBuilder &builder, OperationState &state,
}
LogicalResult WsloopOp::verify() {
+ return verifyReductionVarList(*this, getReductionSyms(), getReductionVars(),
+ getReductionByref());
+}
+
+LogicalResult WsloopOp::verifyRegions() {
bool isCompositeChildLeaf =
llvm::dyn_cast_if_present<LoopWrapperInterface>((*this)->getParentOp());
@@ -2000,8 +2008,7 @@ LogicalResult WsloopOp::verify() {
<< "'omp.composite' attribute missing from composite wrapper";
}
- return verifyReductionVarList(*this, getReductionSyms(), getReductionVars(),
- getReductionByref());
+ return success();
}
//===----------------------------------------------------------------------===//
@@ -2035,9 +2042,6 @@ LogicalResult SimdOp::verify() {
if (verifyNontemporalClause(*this, getNontemporalVars()).failed())
return failure();
- if (getNestedWrapper())
- return emitOpError() << "must wrap an 'omp.loop_nest' directly";
-
bool isCompositeChildLeaf =
llvm::dyn_cast_if_present<LoopWrapperInterface>((*this)->getParentOp());
@@ -2052,6 +2056,13 @@ LogicalResult SimdOp::verify() {
return success();
}
+LogicalResult SimdOp::verifyRegions() {
+ if (getNestedWrapper())
+ return emitOpError() << "must wrap an 'omp.loop_nest' directly";
+
+ return success();
+}
+
//===----------------------------------------------------------------------===//
// Distribute construct [2.9.4.1]
//===----------------------------------------------------------------------===//
@@ -2074,6 +2085,10 @@ LogicalResult DistributeOp::verify() {
return emitError(
"expected equal sizes for allocate and allocator variables");
+ return success();
+}
+
+LogicalResult DistributeOp::verifyRegions() {
if (LoopWrapperInterface nested = getNestedWrapper()) {
if (!isComposite())
return emitError()
@@ -2279,6 +2294,10 @@ LogicalResult TaskloopOp::verify() {
"may not appear on the same taskloop directive");
}
+ return success();
+}
+
+LogicalResult TaskloopOp::verifyRegions() {
if (LoopWrapperInterface nested = getNestedWrapper()) {
if (!isComposite())
return emitError()
@@ -2723,7 +2742,7 @@ void PrivateClauseOp::build(OpBuilder &odsBuilder, OperationState &odsState,
DataSharingClauseType::Private));
}
-LogicalResult PrivateClauseOp::verify() {
+LogicalResult PrivateClauseOp::verifyRegions() {
Type symType = getType();
auto verifyTerminator = [&](Operation *terminator,
diff --git a/mlir/test/Dialect/OpenMP/invalid.mlir b/mlir/test/Dialect/OpenMP/invalid.mlir
index f7a87713aca351..fd89ec31c64a60 100644
--- a/mlir/test/Dialect/OpenMP/invalid.mlir
+++ b/mlir/test/Dialect/OpenMP/invalid.mlir
@@ -136,9 +136,11 @@ func.func @invalid_nested_wrapper(%lb : index, %ub : index, %step : index) {
// expected-error @below {{only supported nested wrapper is 'omp.simd'}}
omp.wsloop {
omp.distribute {
- omp.loop_nest (%iv) : index = (%lb) to (%ub) step (%step) {
- omp.yield
- }
+ omp.simd {
+ omp.loop_nest (%iv) : index = (%lb) to (%ub) step (%step) {
+ omp.yield
+ }
+ } {omp.composite}
} {omp.composite}
} {omp.composite}
}
@@ -1975,7 +1977,7 @@ func.func @taskloop(%lb: i32, %ub: i32, %step: i32) {
omp.loop_nest (%iv) : i32 = (%lb) to (%ub) step (%step) {
omp.yield
}
- } {omp.composite}
+ }
} {omp.composite}
return
}
@@ -2188,7 +2190,7 @@ func.func @omp_distribute_nested_wrapper2(%lb: index, %ub: index, %step: index)
omp.loop_nest (%iv) : index = (%lb) to (%ub) step (%step) {
"omp.yield"() : () -> ()
}
- }) {omp.composite} : () -> ()
+ }) : () -> ()
} {omp.composite}
}
|
tblah
approved these changes
Oct 16, 2024
This patch moves the part of operation verifiers dependent on the contents of their regions to the corresponding `verifyRegions` method. This ensures these are only triggered after the operations in the region have themselved already been verified in advance, avoiding checks based on invalid nested operations. The `LoopWrapperInterface` is also updated so that its verifier runs after operations in the region of ops with this interface have already been verified.
skatrak
force-pushed
the
region-verifiers
branch
from
8180edd to
9cd86a7
Compare
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 the part of operation verifiers dependent on the contents of their regions to the corresponding
verifyRegionsmethod. This ensures these are only triggered after the operations in the region have themselved already been verified in advance, avoiding checks based on invalid nested operations.The
LoopWrapperInterfaceis also updated so that its verifier runs after operations in the region of ops with this interface have already been verified.