[flang][OpenMP] use attribute for delayed privatization barrier #140092
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
A barrier is needed at the end of initialization/copying of private variables if any of those variables is lastprivate. This ensures that all firstprivate variables receive the original value of the variable before the lastprivate clause overwrites it. Previously this barrier was added by the flang fontend, but there is not a reliable way to put the barrier in the correct place for delayed privatization, and the OpenMP dialect could some day have other users. It is important that there are safe ways to use the constructs available in the dialect. lastprivate is currently not modelled in the OpenMP dialect, and so there is no way to reliably determine whether there were lastprivate variables. Therefore the frontend will have to provide this information through this new attribute. Part of a series of patches to fix #136357
Fixes #136357 The barrier needs to go between the copying into firstprivate variables and the initialization call for the OpenMP construct (e.g. wsloop). There is no way of expressing this in MLIR because for delayed privatization that is all implicit (added in MLIR->LLVMIR conversion). The previous approach put the barrier immediately before the wsloop (or similar). For delayed privatization, the firstprivate copy code would then be inserted after that, opening the possibility for the race observed in the bug report. This patch solves the issue by instead setting an attribute on the mlir operation, which will instruct openmp dialect to llvm ir conversion to insert a barrier in the correct place.
llvmbot
added
flang
|
@llvm/pr-subscribers-mlir @llvm/pr-subscribers-flang-openmp Author: Tom Eccles (tblah) ChangesFixes #136357 The barrier needs to go between the copying into firstprivate variables and the initialization call for the OpenMP construct (e.g. wsloop). There is no way of expressing this in MLIR because for delayed privatization that is all implicit (added in MLIR->LLVMIR conversion). The previous approach put the barrier immediately before the wsloop (or similar). For delayed privatization, the firstprivate copy code would then be inserted after that, opening the possibility for the race observed in the bug report. This patch solves the issue by instead setting an attribute on the mlir operation, which will instruct openmp dialect to llvm ir conversion to insert a barrier in the correct place. Full diff: https://github.com/llvm/llvm-project/pull/140092.diff 5 Files Affected:
diff --git a/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp b/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp
index 7eec598645eac..0949fe84f209f 100644
--- a/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp
+++ b/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp
@@ -62,7 +62,7 @@ void DataSharingProcessor::processStep1(
privatize(clauseOps);
- insertBarrier();
+ insertBarrier(clauseOps);
}
void DataSharingProcessor::processStep2(mlir::Operation *op, bool isLoop) {
@@ -230,8 +230,15 @@ bool DataSharingProcessor::needBarrier() {
return false;
}
-void DataSharingProcessor::insertBarrier() {
- if (needBarrier())
+void DataSharingProcessor::insertBarrier(
+ mlir::omp::PrivateClauseOps *clauseOps) {
+ if (!needBarrier())
+ return;
+
+ if (useDelayedPrivatization)
+ clauseOps->privateNeedsBarrier =
+ mlir::UnitAttr::get(&converter.getMLIRContext());
+ else
firOpBuilder.create<mlir::omp::BarrierOp>(converter.getCurrentLocation());
}
diff --git a/flang/lib/Lower/OpenMP/DataSharingProcessor.h b/flang/lib/Lower/OpenMP/DataSharingProcessor.h
index 54a42fd199831..7787e4ffb03c2 100644
--- a/flang/lib/Lower/OpenMP/DataSharingProcessor.h
+++ b/flang/lib/Lower/OpenMP/DataSharingProcessor.h
@@ -100,7 +100,7 @@ class DataSharingProcessor {
const omp::ObjectList &objects,
llvm::SetVector<const semantics::Symbol *> &symbolSet);
void collectSymbolsForPrivatization();
- void insertBarrier();
+ void insertBarrier(mlir::omp::PrivateClauseOps *clauseOps);
void collectDefaultSymbols();
void collectImplicitSymbols();
void collectPreDeterminedSymbols();
diff --git a/flang/test/Lower/OpenMP/lastprivate-allocatable.f90 b/flang/test/Lower/OpenMP/lastprivate-allocatable.f90
index 1d31edd16efea..c2626e14b51c7 100644
--- a/flang/test/Lower/OpenMP/lastprivate-allocatable.f90
+++ b/flang/test/Lower/OpenMP/lastprivate-allocatable.f90
@@ -8,7 +8,7 @@
! CHECK: fir.store %[[VAL_2]] to %[[VAL_0]] : !fir.ref<!fir.box<!fir.heap<i32>>>
! CHECK: %[[VAL_3:.*]]:2 = hlfir.declare %[[VAL_0]] {fortran_attrs = {{.*}}<allocatable>, uniq_name = "_QFEa"} : (!fir.ref<!fir.box<!fir.heap<i32>>>) -> (!fir.ref<!fir.box<!fir.heap<i32>>>, !fir.ref<!fir.box<!fir.heap<i32>>>)
! CHECK: omp.parallel {
-! CHECK: omp.wsloop private(@{{.*}} %{{.*}} -> %{{.*}}, @{{.*}} %{{.*}} -> %[[VAL_17:.*]] : !fir.ref<!fir.box<!fir.heap<i32>>>, !fir.ref<i32>) {
+! CHECK: omp.wsloop private(@{{.*}} %{{.*}} -> %{{.*}}, @{{.*}} %{{.*}} -> %[[VAL_17:.*]] : !fir.ref<!fir.box<!fir.heap<i32>>>, !fir.ref<i32>) private_barrier {
! CHECK: omp.loop_nest
! CHECK: %[[VAL_16:.*]]:2 = hlfir.declare %{{.*}} {fortran_attrs = {{.*}}<allocatable>, uniq_name = "_QFEa"} : (!fir.ref<!fir.box<!fir.heap<i32>>>) -> (!fir.ref<!fir.box<!fir.heap<i32>>>, !fir.ref<!fir.box<!fir.heap<i32>>>)
! CHECK: %[[VAL_18:.*]]:2 = hlfir.declare %[[VAL_17]] {uniq_name = "_QFEi"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
diff --git a/flang/test/Lower/OpenMP/parallel-lastprivate-clause-scalar.f90 b/flang/test/Lower/OpenMP/parallel-lastprivate-clause-scalar.f90
index 60de8fa6f46a2..5d37010f4095b 100644
--- a/flang/test/Lower/OpenMP/parallel-lastprivate-clause-scalar.f90
+++ b/flang/test/Lower/OpenMP/parallel-lastprivate-clause-scalar.f90
@@ -226,8 +226,7 @@ subroutine firstpriv_lastpriv_int(arg1, arg2)
! Firstprivate update
-!CHECK-NEXT: omp.barrier
-!CHECK: omp.wsloop private(@{{.*}} %{{.*}}#0 -> %[[CLONE1:.*]], @{{.*}} %{{.*}}#0 -> %[[IV:.*]] : !fir.ref<i32>, !fir.ref<i32>) {
+!CHECK: omp.wsloop private(@{{.*}} %{{.*}}#0 -> %[[CLONE1:.*]], @{{.*}} %{{.*}}#0 -> %[[IV:.*]] : !fir.ref<i32>, !fir.ref<i32>) private_barrier {
!CHECK-NEXT: omp.loop_nest (%[[INDX_WS:.*]]) : {{.*}} {
!CHECK: %[[CLONE1_DECL:.*]]:2 = hlfir.declare %[[CLONE1]] {uniq_name = "_QFfirstpriv_lastpriv_int2Earg1"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
diff --git a/flang/test/Lower/OpenMP/same_var_first_lastprivate.f90 b/flang/test/Lower/OpenMP/same_var_first_lastprivate.f90
index ee914f23aacf3..45d6f91f67f1f 100644
--- a/flang/test/Lower/OpenMP/same_var_first_lastprivate.f90
+++ b/flang/test/Lower/OpenMP/same_var_first_lastprivate.f90
@@ -20,8 +20,7 @@ subroutine first_and_lastprivate
! CHECK: func.func @{{.*}}first_and_lastprivate()
! CHECK: %[[ORIG_VAR_DECL:.*]]:2 = hlfir.declare %{{.*}} {uniq_name = "{{.*}}Evar"}
! CHECK: omp.parallel {
-! CHECK: omp.barrier
-! CHECK: omp.wsloop private(@{{.*}}var_firstprivate_i32 {{.*}}) {
+! CHECK: omp.wsloop private(@{{.*}}var_firstprivate_i32 {{.*}}) private_barrier {
! CHECK: omp.loop_nest {{.*}} {
! CHECK: %[[PRIV_VAR_DECL:.*]]:2 = hlfir.declare %{{.*}} {uniq_name = "{{.*}}Evar"}
! CHECK: fir.if %{{.*}} {
|
This was referenced May 15, 2025
|
ping for review |
luporl
approved these changes
May 22, 2025
| void DataSharingProcessor::insertBarrier() { | ||
| if (needBarrier()) | ||
| void DataSharingProcessor::insertBarrier( | ||
| mlir::omp::PrivateClauseOps *clauseOps) { |
There was a problem hiding this comment.
Suggested change
| mlir::omp::PrivateClauseOps *clauseOps) { | |
| mlir::omp::PrivateClauseOps &clauseOps) { |
If clauseOps can't be NULL, it would be better to use a reference.
sivan-shani
pushed a commit
to sivan-shani/llvm-project
that referenced
this pull request
Jun 3, 2025
…#140092) Fixes llvm#136357 The barrier needs to go between the copying into firstprivate variables and the initialization call for the OpenMP construct (e.g. wsloop). There is no way of expressing this in MLIR because for delayed privatization that is all implicit (added in MLIR->LLVMIR conversion). The previous approach put the barrier immediately before the wsloop (or similar). For delayed privatization, the firstprivate copy code would then be inserted after that, opening the possibility for the race observed in the bug report. This patch solves the issue by instead setting an attribute on the mlir operation, which will instruct openmp dialect to llvm ir conversion to insert a barrier in the correct place.
ajaden-codes
pushed a commit
to Jaddyen/llvm-project
that referenced
this pull request
Jun 6, 2025
…#140092) Fixes llvm#136357 The barrier needs to go between the copying into firstprivate variables and the initialization call for the OpenMP construct (e.g. wsloop). There is no way of expressing this in MLIR because for delayed privatization that is all implicit (added in MLIR->LLVMIR conversion). The previous approach put the barrier immediately before the wsloop (or similar). For delayed privatization, the firstprivate copy code would then be inserted after that, opening the possibility for the race observed in the bug report. This patch solves the issue by instead setting an attribute on the mlir operation, which will instruct openmp dialect to llvm ir conversion to insert a barrier in the correct place.
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.
Fixes #136357
The barrier needs to go between the copying into firstprivate variables and the initialization call for the OpenMP construct (e.g. wsloop). There is no way of expressing this in MLIR because for delayed privatization that is all implicit (added in MLIR->LLVMIR conversion).
The previous approach put the barrier immediately before the wsloop (or similar). For delayed privatization, the firstprivate copy code would then be inserted after that, opening the possibility for the race observed in the bug report.
This patch solves the issue by instead setting an attribute on the mlir operation, which will instruct openmp dialect to llvm ir conversion to insert a barrier in the correct place.