[flang][OpenMP] Fix location of barrier in copyin clause

[kparzysz]

Insert the barrier after the last executed copy, not the most recently inserted copy.
This fixes #91205.

[llvmbot]

@llvm/pr-subscribers-flang-openmp

@llvm/pr-subscribers-flang-fir-hlfir

Author: Krzysztof Parzyszek (kparzysz)

Changes

Insert the barrier after the last executed copy, not the most recently inserted copy.
This fixes #91205.

---

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

2 Files Affected:

• (modified) flang/lib/Lower/OpenMP/ClauseProcessor.cpp (+8-1)
• (added) flang/test/Lower/OpenMP/copyin-order.f90 (+31)

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 different order in `copyin`.
+  !$omp parallel if (x1 .eq. x2(1)) copyin(x2, x1)
+    call sub1(x1, x2)
+  !$omp end parallel
+
+end
+

[kiranchandramohan]

LGTM.

[luporl]

LGTM.

[luporl]

Suggested change

	! Have x1 appear before x2 in the AST node for the `parallel construct,
	! Have x1 appear before x2 in the AST node for the `parallel` construct,

[luporl]

Nit: I usually expect to find the test description or link to the related issue at the beginning of the test file, right after the RUN line.

[kparzysz]

I'll fix these in a separate commit.