From e9cba3c8edca3dc805e82afbb482b3938cb96ae2 Mon Sep 17 00:00:00 2001 From: Tom Eccles Date: Thu, 22 May 2025 15:25:53 +0100 Subject: [PATCH] [flang][OpenMP] use attribute for delayed privatization barrier (#140092) 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. --- flang/lib/Lower/OpenMP/DataSharingProcessor.cpp | 15 ++++++++++++--- flang/lib/Lower/OpenMP/DataSharingProcessor.h | 2 +- .../test/Lower/OpenMP/lastprivate-allocatable.f90 | 2 +- .../OpenMP/parallel-lastprivate-clause-scalar.f90 | 3 +-- .../Lower/OpenMP/same_var_first_lastprivate.f90 | 3 +-- 5 files changed, 16 insertions(+), 9 deletions(-) diff --git a/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp b/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp index 2a1c94407e1c..20dc46e4710f 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) { @@ -231,9 +231,18 @@ bool DataSharingProcessor::needBarrier() { return false; } -void DataSharingProcessor::insertBarrier() { - if (needBarrier()) +void DataSharingProcessor::insertBarrier( + mlir::omp::PrivateClauseOps *clauseOps) { + if (!needBarrier()) + return; + + if (useDelayedPrivatization) { + if (clauseOps) + clauseOps->privateNeedsBarrier = + mlir::UnitAttr::get(&converter.getMLIRContext()); + } else { firOpBuilder.create(converter.getCurrentLocation()); + } } void DataSharingProcessor::insertLastPrivateCompare(mlir::Operation *op) { diff --git a/flang/lib/Lower/OpenMP/DataSharingProcessor.h b/flang/lib/Lower/OpenMP/DataSharingProcessor.h index 54a42fd19983..7787e4ffb03c 100644 --- a/flang/lib/Lower/OpenMP/DataSharingProcessor.h +++ b/flang/lib/Lower/OpenMP/DataSharingProcessor.h @@ -100,7 +100,7 @@ private: const omp::ObjectList &objects, llvm::SetVector &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 1d31edd16efe..c2626e14b51c 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>> ! CHECK: %[[VAL_3:.*]]:2 = hlfir.declare %[[VAL_0]] {fortran_attrs = {{.*}}, uniq_name = "_QFEa"} : (!fir.ref>>) -> (!fir.ref>>, !fir.ref>>) ! CHECK: omp.parallel { -! CHECK: omp.wsloop private(@{{.*}} %{{.*}} -> %{{.*}}, @{{.*}} %{{.*}} -> %[[VAL_17:.*]] : !fir.ref>>, !fir.ref) { +! CHECK: omp.wsloop private(@{{.*}} %{{.*}} -> %{{.*}}, @{{.*}} %{{.*}} -> %[[VAL_17:.*]] : !fir.ref>>, !fir.ref) private_barrier { ! CHECK: omp.loop_nest ! CHECK: %[[VAL_16:.*]]:2 = hlfir.declare %{{.*}} {fortran_attrs = {{.*}}, uniq_name = "_QFEa"} : (!fir.ref>>) -> (!fir.ref>>, !fir.ref>>) ! CHECK: %[[VAL_18:.*]]:2 = hlfir.declare %[[VAL_17]] {uniq_name = "_QFEi"} : (!fir.ref) -> (!fir.ref, !fir.ref) diff --git a/flang/test/Lower/OpenMP/parallel-lastprivate-clause-scalar.f90 b/flang/test/Lower/OpenMP/parallel-lastprivate-clause-scalar.f90 index 60de8fa6f46a..5d37010f4095 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 @@ end subroutine ! Firstprivate update -!CHECK-NEXT: omp.barrier -!CHECK: omp.wsloop private(@{{.*}} %{{.*}}#0 -> %[[CLONE1:.*]], @{{.*}} %{{.*}}#0 -> %[[IV:.*]] : !fir.ref, !fir.ref) { +!CHECK: omp.wsloop private(@{{.*}} %{{.*}}#0 -> %[[CLONE1:.*]], @{{.*}} %{{.*}}#0 -> %[[IV:.*]] : !fir.ref, !fir.ref) private_barrier { !CHECK-NEXT: omp.loop_nest (%[[INDX_WS:.*]]) : {{.*}} { !CHECK: %[[CLONE1_DECL:.*]]:2 = hlfir.declare %[[CLONE1]] {uniq_name = "_QFfirstpriv_lastpriv_int2Earg1"} : (!fir.ref) -> (!fir.ref, !fir.ref) diff --git a/flang/test/Lower/OpenMP/same_var_first_lastprivate.f90 b/flang/test/Lower/OpenMP/same_var_first_lastprivate.f90 index ee914f23aacf..45d6f91f67f1 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 @@ end subroutine ! 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 %{{.*}} {