[mlir][OpenMP] Don't allow firstprivate for simd #146734
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
This is not allowed by the openmp standard.
|
@llvm/pr-subscribers-mlir-llvm @llvm/pr-subscribers-mlir-openmp Author: Tom Eccles (tblah) ChangesThis is not allowed by the openmp standard. Full diff: https://github.com/llvm/llvm-project/pull/146734.diff 2 Files Affected:
diff --git a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
index 7b07243c5f843..b307de09fbcc4 100644
--- a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
+++ b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
@@ -326,6 +326,25 @@ static LogicalResult checkImplementationStatus(Operation &op) {
if (op.getDistScheduleChunkSize())
result = todo("dist_schedule with chunk_size");
};
+ auto checkFirstprivate = [&todo](auto op, LogicalResult &result) {
+ // Firstprivate is not listed as supported by the simd operation in
+ // OpenMP 6.0. This is here to catch it because it is allowed at the dialect
+ // level.
+ if constexpr (std::is_same_v<std::decay_t<decltype(op)>, omp::SimdOp>) {
+ std::optional<ArrayAttr> privateSyms = op.getPrivateSyms();
+ if (!privateSyms)
+ return;
+ for (const Attribute &sym : *privateSyms) {
+ omp::PrivateClauseOp privatizer =
+ findPrivatizer(op, cast<SymbolRefAttr>(sym));
+ if (privatizer.getDataSharingType() ==
+ omp::DataSharingClauseType::FirstPrivate) {
+ result = todo("firstprivate");
+ break;
+ }
+ }
+ }
+ };
auto checkHint = [](auto op, LogicalResult &) {
if (op.getHint())
op.emitWarning("hint clause discarded");
@@ -441,6 +460,7 @@ static LogicalResult checkImplementationStatus(Operation &op) {
checkReduction(op, result);
})
.Case([&](omp::SimdOp op) {
+ checkFirstprivate(op, result);
checkLinear(op, result);
checkReduction(op, result);
})
@@ -2894,7 +2914,8 @@ convertOmpSimd(Operation &opInst, llvm::IRBuilderBase &builder,
.failed())
return failure();
- // TODO: no call to copyFirstPrivateVars?
+ // No call to copyFirstPrivateVars because firstprivate is not allowed on
+ // SIMD.
assert(afterAllocas.get()->getSinglePredecessor());
if (failed(initReductionVars(simdOp, reductionArgs, builder,
diff --git a/mlir/test/Target/LLVMIR/openmp-todo.mlir b/mlir/test/Target/LLVMIR/openmp-todo.mlir
index 29725a02c075a..e86a91dab873d 100644
--- a/mlir/test/Target/LLVMIR/openmp-todo.mlir
+++ b/mlir/test/Target/LLVMIR/openmp-todo.mlir
@@ -503,3 +503,26 @@ llvm.func @wsloop_order(%lb : i32, %ub : i32, %step : i32) {
}
llvm.return
}
+
+// -----
+
+omp.private {type = firstprivate} @_QFEp_firstprivate_i32 : i32 copy {
+^bb0(%arg0: !llvm.ptr, %arg1: !llvm.ptr):
+ %0 = llvm.load %arg0 : !llvm.ptr -> i32
+ llvm.store %0, %arg1 : i32, !llvm.ptr
+ omp.yield(%arg1 : !llvm.ptr)
+}
+llvm.func @_QQmain() {
+ %0 = llvm.mlir.constant(1 : i64) : i64
+ %1 = llvm.alloca %0 x i32 : (i64) -> !llvm.ptr
+ %3 = llvm.mlir.constant(10 : i32) : i32
+ %4 = llvm.mlir.constant(1 : i32) : i32
+ // expected-error@below {{not yet implemented: Unhandled clause firstprivate in omp.simd operation}}
+ // expected-error@below {{LLVM Translation failed for operation: omp.simd}}
+ omp.simd private(@_QFEp_firstprivate_i32 %1 -> %arg0 : !llvm.ptr) {
+ omp.loop_nest (%arg2) : i32 = (%4) to (%3) inclusive step (%4) {
+ omp.yield
+ }
+ }
+ llvm.return
+}
|
|
@llvm/pr-subscribers-flang-openmp Author: Tom Eccles (tblah) ChangesThis is not allowed by the openmp standard. Full diff: https://github.com/llvm/llvm-project/pull/146734.diff 2 Files Affected:
diff --git a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
index 7b07243c5f843..b307de09fbcc4 100644
--- a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
+++ b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
@@ -326,6 +326,25 @@ static LogicalResult checkImplementationStatus(Operation &op) {
if (op.getDistScheduleChunkSize())
result = todo("dist_schedule with chunk_size");
};
+ auto checkFirstprivate = [&todo](auto op, LogicalResult &result) {
+ // Firstprivate is not listed as supported by the simd operation in
+ // OpenMP 6.0. This is here to catch it because it is allowed at the dialect
+ // level.
+ if constexpr (std::is_same_v<std::decay_t<decltype(op)>, omp::SimdOp>) {
+ std::optional<ArrayAttr> privateSyms = op.getPrivateSyms();
+ if (!privateSyms)
+ return;
+ for (const Attribute &sym : *privateSyms) {
+ omp::PrivateClauseOp privatizer =
+ findPrivatizer(op, cast<SymbolRefAttr>(sym));
+ if (privatizer.getDataSharingType() ==
+ omp::DataSharingClauseType::FirstPrivate) {
+ result = todo("firstprivate");
+ break;
+ }
+ }
+ }
+ };
auto checkHint = [](auto op, LogicalResult &) {
if (op.getHint())
op.emitWarning("hint clause discarded");
@@ -441,6 +460,7 @@ static LogicalResult checkImplementationStatus(Operation &op) {
checkReduction(op, result);
})
.Case([&](omp::SimdOp op) {
+ checkFirstprivate(op, result);
checkLinear(op, result);
checkReduction(op, result);
})
@@ -2894,7 +2914,8 @@ convertOmpSimd(Operation &opInst, llvm::IRBuilderBase &builder,
.failed())
return failure();
- // TODO: no call to copyFirstPrivateVars?
+ // No call to copyFirstPrivateVars because firstprivate is not allowed on
+ // SIMD.
assert(afterAllocas.get()->getSinglePredecessor());
if (failed(initReductionVars(simdOp, reductionArgs, builder,
diff --git a/mlir/test/Target/LLVMIR/openmp-todo.mlir b/mlir/test/Target/LLVMIR/openmp-todo.mlir
index 29725a02c075a..e86a91dab873d 100644
--- a/mlir/test/Target/LLVMIR/openmp-todo.mlir
+++ b/mlir/test/Target/LLVMIR/openmp-todo.mlir
@@ -503,3 +503,26 @@ llvm.func @wsloop_order(%lb : i32, %ub : i32, %step : i32) {
}
llvm.return
}
+
+// -----
+
+omp.private {type = firstprivate} @_QFEp_firstprivate_i32 : i32 copy {
+^bb0(%arg0: !llvm.ptr, %arg1: !llvm.ptr):
+ %0 = llvm.load %arg0 : !llvm.ptr -> i32
+ llvm.store %0, %arg1 : i32, !llvm.ptr
+ omp.yield(%arg1 : !llvm.ptr)
+}
+llvm.func @_QQmain() {
+ %0 = llvm.mlir.constant(1 : i64) : i64
+ %1 = llvm.alloca %0 x i32 : (i64) -> !llvm.ptr
+ %3 = llvm.mlir.constant(10 : i32) : i32
+ %4 = llvm.mlir.constant(1 : i32) : i32
+ // expected-error@below {{not yet implemented: Unhandled clause firstprivate in omp.simd operation}}
+ // expected-error@below {{LLVM Translation failed for operation: omp.simd}}
+ omp.simd private(@_QFEp_firstprivate_i32 %1 -> %arg0 : !llvm.ptr) {
+ omp.loop_nest (%arg2) : i32 = (%4) to (%3) inclusive step (%4) {
+ omp.yield
+ }
+ }
+ llvm.return
+}
|
skatrak
reviewed
Jul 2, 2025
| if (op.getDistScheduleChunkSize()) | ||
| result = todo("dist_schedule with chunk_size"); | ||
| }; | ||
| auto checkFirstprivate = [&todo](auto op, LogicalResult &result) { |
There was a problem hiding this comment.
Rather than checking this here, perhaps the MLIR op verifier (SimdOp::verify) would be a better place to do it. Otherwise, if a frontend produced an invalid MLIR where omp.simd took a firstprivate privatizer argument, it would be reported as not-yet-implemented when it's actually an invalid condition.
skatrak
approved these changes
Jul 4, 2025
|
|
||
| // TODO: no call to copyFirstPrivateVars? | ||
| // No call to copyFirstPrivateVars because FIRSTPRIVATE is not allowed for | ||
| // SIMD |
There was a problem hiding this comment.
Suggested change
| // SIMD | |
| // SIMD. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/153/builds/36959 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/157/builds/32743 Here is the relevant piece of the build log for the reference |
tblah
added a commit
to tblah/llvm-project
that referenced
this pull request
Jul 4, 2025
These tests referred to privatizers which were never declared
tblah
added a commit
that referenced
this pull request
Jul 4, 2025
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 is not allowed by the openmp standard.