Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 6, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion flang/lib/Lower/OpenMP/ClauseProcessor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
31 changes: 31 additions & 0 deletions flang/test/Lower/OpenMP/copyin-order.f90
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll fix these in a separate commit.


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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,

! 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