Skip to content

Commit 89db24e

Browse files
luporlAlexisPerry
luporl
authored and
AlexisPerry
committed
[flang][OpenMP] Fix copyprivate allocatable/pointer lowering (llvm#95975)
The lowering of copyprivate clauses with allocatable or pointer variables was incorrect. This happened because the values passed to copyVar() are always wrapped in SymbolBox::Intrinsic, which resulted in allocatable/pointer variables being handled as regular ones. This is fixed by providing to copyVar() the attributes of the variables being copied, to make it possible to detect and handle allocatable/pointer variables correctly. Fixes llvm#95801
1 parent b0bc32c commit 89db24e

File tree

4 files changed

+93
-22
lines changed

4 files changed

+93
-22
lines changed

‎flang/include/flang/Lower/AbstractConverter.h

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
#include "flang/Lower/LoweringOptions.h"
1818
#include "flang/Lower/PFTDefs.h"
1919
#include "flang/Optimizer/Builder/BoxValue.h"
20+
#include "flang/Optimizer/Dialect/FIRAttr.h"
2021
#include "flang/Semantics/symbol.h"
2122
#include "mlir/IR/Builders.h"
2223
#include "mlir/IR/BuiltinOps.h"
@@ -126,8 +127,8 @@ class AbstractConverter {
126127
const Fortran::semantics::Symbol &sym,
127128
mlir::OpBuilder::InsertPoint *copyAssignIP = nullptr) = 0;
128129

129-
virtual void copyVar(mlir::Location loc, mlir::Value dst,
130-
mlir::Value src) = 0;
130+
virtual void copyVar(mlir::Location loc, mlir::Value dst, mlir::Value src,
131+
fir::FortranVariableFlagsEnum attrs) = 0;
131132

132133
/// For a given symbol, check if it is present in the inner-most
133134
/// level of the symbol map.

‎flang/lib/Lower/Bridge.cpp

Lines changed: 33 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -757,10 +757,16 @@ class FirConverter : public Fortran::lower::AbstractConverter {
757757
});
758758
}
759759

760-
void copyVar(mlir::Location loc, mlir::Value dst,
761-
mlir::Value src) override final {
760+
void copyVar(mlir::Location loc, mlir::Value dst, mlir::Value src,
761+
fir::FortranVariableFlagsEnum attrs) override final {
762+
bool isAllocatable =
763+
bitEnumContainsAny(attrs, fir::FortranVariableFlagsEnum::allocatable);
764+
bool isPointer =
765+
bitEnumContainsAny(attrs, fir::FortranVariableFlagsEnum::pointer);
766+
762767
copyVarHLFIR(loc, Fortran::lower::SymbolBox::Intrinsic{dst},
763-
Fortran::lower::SymbolBox::Intrinsic{src});
768+
Fortran::lower::SymbolBox::Intrinsic{src}, isAllocatable,
769+
isPointer);
764770
}
765771

766772
void copyHostAssociateVar(
@@ -1089,6 +1095,28 @@ class FirConverter : public Fortran::lower::AbstractConverter {
10891095
void copyVarHLFIR(mlir::Location loc, Fortran::lower::SymbolBox dst,
10901096
Fortran::lower::SymbolBox src) {
10911097
assert(lowerToHighLevelFIR());
1098+
1099+
bool isBoxAllocatable = dst.match(
1100+
[](const fir::MutableBoxValue &box) { return box.isAllocatable(); },
1101+
[](const fir::FortranVariableOpInterface &box) {
1102+
return fir::FortranVariableOpInterface(box).isAllocatable();
1103+
},
1104+
[](const auto &box) { return false; });
1105+
1106+
bool isBoxPointer = dst.match(
1107+
[](const fir::MutableBoxValue &box) { return box.isPointer(); },
1108+
[](const fir::FortranVariableOpInterface &box) {
1109+
return fir::FortranVariableOpInterface(box).isPointer();
1110+
},
1111+
[](const auto &box) { return false; });
1112+
1113+
copyVarHLFIR(loc, dst, src, isBoxAllocatable, isBoxPointer);
1114+
}
1115+
1116+
void copyVarHLFIR(mlir::Location loc, Fortran::lower::SymbolBox dst,
1117+
Fortran::lower::SymbolBox src, bool isAllocatable,
1118+
bool isPointer) {
1119+
assert(lowerToHighLevelFIR());
10921120
hlfir::Entity lhs{dst.getAddr()};
10931121
hlfir::Entity rhs{src.getAddr()};
10941122
// Temporary_lhs is set to true in hlfir.assign below to avoid user
@@ -1105,21 +1133,7 @@ class FirConverter : public Fortran::lower::AbstractConverter {
11051133
/*temporary_lhs=*/true);
11061134
};
11071135

1108-
bool isBoxAllocatable = dst.match(
1109-
[](const fir::MutableBoxValue &box) { return box.isAllocatable(); },
1110-
[](const fir::FortranVariableOpInterface &box) {
1111-
return fir::FortranVariableOpInterface(box).isAllocatable();
1112-
},
1113-
[](const auto &box) { return false; });
1114-
1115-
bool isBoxPointer = dst.match(
1116-
[](const fir::MutableBoxValue &box) { return box.isPointer(); },
1117-
[](const fir::FortranVariableOpInterface &box) {
1118-
return fir::FortranVariableOpInterface(box).isPointer();
1119-
},
1120-
[](const auto &box) { return false; });
1121-
1122-
if (isBoxAllocatable) {
1136+
if (isAllocatable) {
11231137
// Deep copy allocatable if it is allocated.
11241138
// Note that when allocated, the RHS is already allocated with the LHS
11251139
// shape for copy on entry in createHostAssociateVarClone.
@@ -1134,7 +1148,7 @@ class FirConverter : public Fortran::lower::AbstractConverter {
11341148
copyData(lhs, rhs);
11351149
})
11361150
.end();
1137-
} else if (isBoxPointer) {
1151+
} else if (isPointer) {
11381152
// Set LHS target to the target of RHS (do not copy the RHS
11391153
// target data into the LHS target storage).
11401154
auto loadVal = builder->create<fir::LoadOp>(loc, rhs);

‎flang/lib/Lower/OpenMP/ClauseProcessor.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -729,7 +729,7 @@ createCopyFunc(mlir::Location loc, lower::AbstractConverter &converter,
729729
auto declSrc = builder.create<hlfir::DeclareOp>(
730730
loc, funcOp.getArgument(1), copyFuncName + "_src", shape, typeparams,
731731
/*dummy_scope=*/nullptr, attrs);
732-
converter.copyVar(loc, declDst.getBase(), declSrc.getBase());
732+
converter.copyVar(loc, declDst.getBase(), declSrc.getBase(), varAttrs);
733733
builder.create<mlir::func::ReturnOp>(loc);
734734
return funcOp;
735735
}

‎flang/test/Lower/OpenMP/copyprivate2.f90

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
! Test lowering of COPYPRIVATE with allocatable/pointer variables.
2+
! RUN: %flang_fc1 -emit-hlfir -fopenmp -o - %s 2>&1 | FileCheck %s
3+
4+
!CHECK-LABEL: func private @_copy_box_ptr_i32(
5+
!CHECK-SAME: %[[ARG0:.*]]: !fir.ref<!fir.box<!fir.ptr<i32>>>,
6+
!CHECK-SAME: %[[ARG1:.*]]: !fir.ref<!fir.box<!fir.ptr<i32>>>) {
7+
!CHECK-NEXT: %[[DST:.*]]:2 = hlfir.declare %[[ARG0]] {fortran_attrs = #fir.var_attrs<pointer>,
8+
!CHECK-SAME: uniq_name = "_copy_box_ptr_i32_dst"} : (!fir.ref<!fir.box<!fir.ptr<i32>>>) ->
9+
!CHECK-SAME: (!fir.ref<!fir.box<!fir.ptr<i32>>>, !fir.ref<!fir.box<!fir.ptr<i32>>>)
10+
!CHECK-NEXT: %[[SRC:.*]]:2 = hlfir.declare %[[ARG1]] {fortran_attrs = #fir.var_attrs<pointer>,
11+
!CHECK-SAME: uniq_name = "_copy_box_ptr_i32_src"} : (!fir.ref<!fir.box<!fir.ptr<i32>>>) ->
12+
!CHECK-SAME: (!fir.ref<!fir.box<!fir.ptr<i32>>>, !fir.ref<!fir.box<!fir.ptr<i32>>>)
13+
!CHECK-NEXT: %[[SRC_VAL:.*]] = fir.load %[[SRC]]#0 : !fir.ref<!fir.box<!fir.ptr<i32>>>
14+
!CHECK-NEXT: fir.store %[[SRC_VAL]] to %[[DST]]#0 : !fir.ref<!fir.box<!fir.ptr<i32>>>
15+
!CHECK-NEXT: return
16+
!CHECK-NEXT: }
17+
18+
!CHECK-LABEL: func private @_copy_box_heap_Uxi32(
19+
!CHECK-SAME: %[[ARG0:.*]]: !fir.ref<!fir.box<!fir.heap<!fir.array<?xi32>>>>,
20+
!CHECK-SAME: %[[ARG1:.*]]: !fir.ref<!fir.box<!fir.heap<!fir.array<?xi32>>>>) {
21+
!CHECK-NEXT: %[[DST:.*]]:2 = hlfir.declare %[[ARG0]] {fortran_attrs = #fir.var_attrs<allocatable>,
22+
!CHECK-SAME: uniq_name = "_copy_box_heap_Uxi32_dst"} : (!fir.ref<!fir.box<!fir.heap<!fir.array<?xi32>>>>) ->
23+
!CHECK-SAME: (!fir.ref<!fir.box<!fir.heap<!fir.array<?xi32>>>>, !fir.ref<!fir.box<!fir.heap<!fir.array<?xi32>>>>)
24+
!CHECK-NEXT: %[[SRC:.*]]:2 = hlfir.declare %[[ARG1]] {fortran_attrs = #fir.var_attrs<allocatable>,
25+
!CHECK-SAME: uniq_name = "_copy_box_heap_Uxi32_src"} : (!fir.ref<!fir.box<!fir.heap<!fir.array<?xi32>>>>) ->
26+
!CHECK-SAME: (!fir.ref<!fir.box<!fir.heap<!fir.array<?xi32>>>>, !fir.ref<!fir.box<!fir.heap<!fir.array<?xi32>>>>)
27+
!CHECK-NEXT: %[[DST_BOX:.*]] = fir.load %[[DST]]#0 : !fir.ref<!fir.box<!fir.heap<!fir.array<?xi32>>>>
28+
!CHECK: fir.if %{{.*}} {
29+
!CHECK-NEXT: %[[SRC_BOX:.*]] = fir.load %[[SRC]]#0 : !fir.ref<!fir.box<!fir.heap<!fir.array<?xi32>>>>
30+
!CHECK-NEXT: hlfir.assign %[[SRC_BOX]] to %[[DST_BOX]] temporary_lhs : !fir.box<!fir.heap<!fir.array<?xi32>>>,
31+
!CHECK-SAME: !fir.box<!fir.heap<!fir.array<?xi32>>>
32+
!CHECK-NEXT: }
33+
!CHECK-NEXT: return
34+
!CHECK-NEXT: }
35+
36+
!CHECK-LABEL: func @_QPtest_alloc_ptr
37+
!CHECK: omp.parallel {
38+
!CHECK: %[[A:.*]]:2 = hlfir.declare %{{.*}} {fortran_attrs = #fir.var_attrs<allocatable>,
39+
!CHECK-SAME: uniq_name = "_QFtest_alloc_ptrEa"} : (!fir.ref<!fir.box<!fir.heap<!fir.array<?xi32>>>>) ->
40+
!CHECK-SAME: (!fir.ref<!fir.box<!fir.heap<!fir.array<?xi32>>>>, !fir.ref<!fir.box<!fir.heap<!fir.array<?xi32>>>>)
41+
!CHECK: %[[P:.*]]:2 = hlfir.declare %{{.*}} {fortran_attrs = #fir.var_attrs<pointer>,
42+
!CHECK-SAME: uniq_name = "_QFtest_alloc_ptrEp"} : (!fir.ref<!fir.box<!fir.ptr<i32>>>) ->
43+
!CHECK-SAME: (!fir.ref<!fir.box<!fir.ptr<i32>>>, !fir.ref<!fir.box<!fir.ptr<i32>>>)
44+
!CHECK: omp.single copyprivate(
45+
!CHECK-SAME: %[[A]]#0 -> @_copy_box_heap_Uxi32 : !fir.ref<!fir.box<!fir.heap<!fir.array<?xi32>>>>,
46+
!CHECK-SAME: %[[P]]#0 -> @_copy_box_ptr_i32 : !fir.ref<!fir.box<!fir.ptr<i32>>>)
47+
!CHEK: }
48+
subroutine test_alloc_ptr()
49+
integer, allocatable :: a(:)
50+
integer, pointer :: p
51+
52+
!$omp parallel private(a, p)
53+
!$omp single
54+
!$omp end single copyprivate(a, p)
55+
!$omp end parallel
56+
end subroutine

0 commit comments

Comments
 (0)