[flang-commits] [flang] 94204f5 - [flang][OpenMP] Fix location of `barrier` in `copyin` clause (#91214)

[via flang-commits]

Author: Krzysztof Parzyszek
Date: 2024-05-06T14:42:55-05:00
New Revision: 94204f59e92473bb19333f40a2250b64a398191a

URL: https://github.com/llvm/llvm-project/commit/94204f59e92473bb19333f40a2250b64a398191a
DIFF: https://github.com/llvm/llvm-project/commit/94204f59e92473bb19333f40a2250b64a398191a.diff

LOG: [flang][OpenMP] Fix location of `barrier` in `copyin` clause (#91214)

Insert the barrier after the last _executed_ copy, not the most recently
inserted copy.
This fixes https://github.com/llvm/llvm-project/issues/91205.

Added: 
    flang/test/Lower/OpenMP/copyin-order.f90

Modified: 
    flang/lib/Lower/OpenMP/ClauseProcessor.cpp

Removed: 
    


################################################################################
diff  --git a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
index 79525d6dfe7a21..bb83e8d5e78889 100644
--- a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
+++ b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
@@ -555,9 +555,16 @@ bool ClauseProcessor::processCopyin() const {
   // synchronize threads and avoid data races on propagation master's thread
   // values of threadprivate variables to local instances of that variables of
   // all other implicit threads.
+
+  // All copies are inserted at either "insPt" (i.e. immediately before it),
+  // or at some earlier point (as determined by "copyHostAssociateVar").
+  // Unless the insertion point is given to "copyHostAssociateVar" explicitly,
+  // it will not restore the builder's insertion point. Since the copies may be
+  // inserted in any order (not following the execution order), make sure the
+  // barrier is inserted following all of them.
+  firOpBuilder.restoreInsertionPoint(insPt);
   if (hasCopyin)
     firOpBuilder.create<mlir::omp::BarrierOp>(converter.getCurrentLocation());
-  firOpBuilder.restoreInsertionPoint(insPt);
   return hasCopyin;
 }
 

diff  --git a/flang/test/Lower/OpenMP/copyin-order.f90 b/flang/test/Lower/OpenMP/copyin-order.f90
new file mode 100644
index 00000000000000..9dcb6e6e54d088
--- /dev/null
+++ b/flang/test/Lower/OpenMP/copyin-order.f90
@@ -0,0 +1,31 @@
+!RUN: bbc -fopenmp -emit-hlfir -o - %s | FileCheck %s
+
+!CHECK: omp.parallel if(%{{[0-9]+}} : i1) {
+!CHECK:   %[[THP1:[0-9]+]] = omp.threadprivate %{{[0-9]+}}#1
+!CHECK:   %[[DCL1:[0-9]+]]:2 = hlfir.declare %[[THP1]] {uniq_name = "_QFcopyin_scalar_arrayEx1"}
+!CHECK:   %[[LD1:[0-9]+]] = fir.load %{{[0-9]+}}#0
+!CHECK:   hlfir.assign %[[LD1]] to %[[DCL1]]#0 temporary_lhs
+!CHECK:   %[[THP2:[0-9]+]] = omp.threadprivate %{{[0-9]+}}#1
+!CHECK:   %[[SHP2:[0-9]+]] = fir.shape %c{{[0-9]+}}
+!CHECK:   %[[DCL2:[0-9]+]]:2 = hlfir.declare %[[THP2]](%[[SHP2]]) {uniq_name = "_QFcopyin_scalar_arrayEx2"}
+!CHECK:   hlfir.assign %{{[0-9]+}}#0 to %[[DCL2]]#0 temporary_lhs
+!CHECK:   omp.barrier
+!CHECK:   fir.call @_QPsub1(%[[DCL1]]#1, %[[DCL2]]#1)
+!CHECK:   omp.terminator
+!CHECK: }
+
+!https://github.com/llvm/llvm-project/issues/91205
+
+subroutine copyin_scalar_array()
+  integer(kind=4), save :: x1
+  integer(kind=8), save :: x2(10)
+  !$omp threadprivate(x1, x2)
+
+  ! Have x1 appear before x2 in the AST node for the `parallel construct,
+  ! but at the same time have them in a 
diff erent order in `copyin`.
+  !$omp parallel if (x1 .eq. x2(1)) copyin(x2, x1)
+    call sub1(x1, x2)
+  !$omp end parallel
+
+end
+