Skip to content

Commit

Permalink
[flang][OpenMP] Fix privatization when critical is present (llvm#94441)
Browse files Browse the repository at this point in the history
When a critical construct is present inside another construct where
privatizations may occur, such as a parallel construct, some
privatizations are skipped if the corresponding symbols are defined
inside the critical section only (see the example below).

This happens because, while critical constructs have a "body", they
don't have a separate scope (which makes sense, since no
privatizations can occur in them). Because of this, in semantics
phase, it's not possible to insert a new host association symbol,
but instead the symbol from the enclosing context is used directly.

This makes symbol collection in DataSharingProcessor consider the
new symbol to be defined by the critical construct, instead of by
the enclosing one, which causes the privatization to be skipped.

Example:
```
!$omp parallel default(firstprivate)
  !$omp critical
     i = 200
  !$omp end critical
!$omp end parallel
```

This patch fixes this by identifying constructs where
privatizations may not happen and skipping them during the
collection of nested symbols. Currently, this seems to happen only
with critical constructs, but others can be easily added to the
skip list, if needed.

Fixes llvm#75767
  • Loading branch information
luporl authored Jun 6, 2024
1 parent c2e62c7 commit 6c9bce8
Show file tree
Hide file tree
Showing 2 changed files with 27 additions and 1 deletion.
4 changes: 3 additions & 1 deletion flang/lib/Lower/OpenMP/DataSharingProcessor.h
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,9 @@ class DataSharingProcessor {
void Post(const T &) {}

bool Pre(const parser::OpenMPConstruct &omp) {
currentConstruct = &omp;
// Skip constructs that may not have privatizations.
if (!std::holds_alternative<parser::OpenMPCriticalConstruct>(omp.u))
currentConstruct = &omp;
return true;
}

Expand Down
24 changes: 24 additions & 0 deletions flang/test/Lower/OpenMP/critical.f90
Original file line number Diff line number Diff line change
Expand Up @@ -51,3 +51,27 @@ subroutine predetermined_privatization()
end do
!$omp end parallel do
end

! https://github.com/llvm/llvm-project/issues/75767
!CHECK-LABEL: func @_QPparallel_critical_privatization(
subroutine parallel_critical_privatization()
integer :: i

!CHECK: %[[I:.*]] = fir.alloca i32 {bindc_name = "i", uniq_name = "_QFparallel_critical_privatizationEi"}
!CHECK: %[[I_DECL:.*]]:2 = hlfir.declare %[[I]] {uniq_name = "_QFparallel_critical_privatizationEi"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
!CHECK: omp.parallel {
!CHECK: %[[PRIV_I:.*]] = fir.alloca i32 {bindc_name = "i", pinned, uniq_name = "_QFparallel_critical_privatizationEi"}
!CHECK: %[[PRIV_I_DECL:.*]]:2 = hlfir.declare %[[PRIV_I]] {uniq_name = "_QFparallel_critical_privatizationEi"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
!CHECK: %[[TEMP:.*]] = fir.load %[[I_DECL]]#0 : !fir.ref<i32>
!CHECK: hlfir.assign %[[TEMP]] to %[[PRIV_I_DECL]]#0 temporary_lhs : i32, !fir.ref<i32>
!$omp parallel default(firstprivate)
!CHECK: omp.critical {
!$omp critical
!CHECK: %[[C200:.*]] = arith.constant 200 : i32
!CHECK: hlfir.assign %[[C200]] to %[[PRIV_I_DECL]]#0 : i32, !fir.ref<i32>
i = 200
!CHECK: }
!$omp end critical
!CHECK: }
!$omp end parallel
end subroutine

0 comments on commit 6c9bce8

Please sign in to comment.