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

[Krzysztof Parzyszek via flang-commits]

https://github.com/kparzysz created https://github.com/llvm/llvm-project/pull/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.

>From 9441e7a06ced906f5647142d030cf723ec5bab2f Mon Sep 17 00:00:00 2001
From: Krzysztof Parzyszek <Krzysztof.Parzyszek at amd.com>
Date: Mon, 6 May 2024 09:17:36 -0500
Subject: [PATCH] [flang][OpenMP] Fix location of `barrier` in `copyin` clause

Insert the barrier after the last _executed_ copy, not the most recently
inserted copy.
This fixes https://github.com/llvm/llvm-project/issues/91205.
---
 flang/lib/Lower/OpenMP/ClauseProcessor.cpp |  9 ++++++-
 flang/test/Lower/OpenMP/copyin-order.f90   | 31 ++++++++++++++++++++++
 2 files changed, 39 insertions(+), 1 deletion(-)
 create mode 100644 flang/test/Lower/OpenMP/copyin-order.f90

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
+