[llvm-branch-commits] [flang] [flang][OpenMP] use attribute for delayed privatization barrier (PR #140092)

[via llvm-branch-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-flang-fir-hlfir

@llvm/pr-subscribers-flang-openmp

Author: Tom Eccles (tblah)

<details>
<summary>Changes</summary>

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.

---
Full diff: https://github.com/llvm/llvm-project/pull/140092.diff


5 Files Affected:

- (modified) flang/lib/Lower/OpenMP/DataSharingProcessor.cpp (+10-3) 
- (modified) flang/lib/Lower/OpenMP/DataSharingProcessor.h (+1-1) 
- (modified) flang/test/Lower/OpenMP/lastprivate-allocatable.f90 (+1-1) 
- (modified) flang/test/Lower/OpenMP/parallel-lastprivate-clause-scalar.f90 (+1-2) 
- (modified) flang/test/Lower/OpenMP/same_var_first_lastprivate.f90 (+1-2) 


``````````diff
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 %{{.*}} {

``````````

</details>


https://github.com/llvm/llvm-project/pull/140092