[flang][OpenMP] use attribute for delayed privatization barrier #140092
+14
−9
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
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-flang-fir-hlfir @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 |
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.