Skip to content

Commit c62761e

Browse files
tblahtblah
committed
[flang][OpenMP] fix lastprivate for allocatables
1) Do not deallocate allocatable private variables if they are lastprivate at the end of the openmp region because their address will have been transferred to the host variable when the variable is copied. 2) Don't use copyHostAssociateVar for allocatable variables. It isn't clear to me whether or not this should be addressed in copyHostAssociateVar instead of inside OpenMP. I opted for OpenMP to minimise how many things I effected. copyHostAssociateVar will not update the destination variable if the destination variable was unallocated. This is incorrect because assignment inside of the openmp block can cause the allocation status of the variable to change. Furthermore, copyHostAssociateVar seems to only copy the variable address not other metadata like the size of the allocation. Reallocation by assignment could cause this to change.
1 parent a8b90c8 commit c62761e

File tree

2 files changed

+91
-3
lines changed

2 files changed

+91
-3
lines changed

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

Lines changed: 47 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,8 @@ void DataSharingProcessor::processStep2(mlir::Operation *op, bool isLoop) {
7979

8080
void DataSharingProcessor::insertDeallocs() {
8181
for (const semantics::Symbol *sym : allPrivatizedSymbols)
82-
if (semantics::IsAllocatable(sym->GetUltimate())) {
82+
if (semantics::IsAllocatable(sym->GetUltimate()) &&
83+
!sym->test(semantics::Symbol::Flag::OmpLastPrivate)) {
8384
if (!useDelayedPrivatization) {
8485
converter.createHostAssociateVarCloneDealloc(*sym);
8586
continue;
@@ -127,8 +128,51 @@ void DataSharingProcessor::copyFirstPrivateSymbol(
127128
void DataSharingProcessor::copyLastPrivateSymbol(
128129
const semantics::Symbol *sym,
129130
[[maybe_unused]] mlir::OpBuilder::InsertPoint *lastPrivIP) {
130-
if (sym->test(semantics::Symbol::Flag::OmpLastPrivate))
131-
converter.copyHostAssociateVar(*sym, lastPrivIP);
131+
if (sym->test(semantics::Symbol::Flag::OmpLastPrivate)) {
132+
bool allocatable = semantics::IsAllocatable(sym->GetUltimate());
133+
if (!allocatable) {
134+
converter.copyHostAssociateVar(*sym, lastPrivIP);
135+
return;
136+
}
137+
138+
// copyHostAssociateVar doesn't work properly if the privatised copy was
139+
// reallocated (e.g. by assignment): it will only copy if the ultimate
140+
// symbol was already allocated, and it only copies data so any reallocated
141+
// lengths etc are lost
142+
143+
// 1) Fetch the original copy of the variable.
144+
assert(sym->has<Fortran::semantics::HostAssocDetails>() &&
145+
"No host-association found");
146+
const Fortran::semantics::Symbol &hsym = sym->GetUltimate();
147+
Fortran::lower::SymbolBox hsb = symTable->lookupOneLevelUpSymbol(hsym);
148+
assert(hsb && "Host symbol box not found");
149+
150+
// 2) Fetch the copied one that will mask the original.
151+
Fortran::lower::SymbolBox sb = symTable->shallowLookupSymbol(sym);
152+
assert(sb && "Host-associated symbol box not found");
153+
assert(hsb.getAddr() != sb.getAddr() &&
154+
"Host and associated symbol boxes are the same");
155+
156+
// 3) Perform the assignment.
157+
fir::FirOpBuilder &builder = converter.getFirOpBuilder();
158+
mlir::Location loc = converter.genLocation(sym->name());
159+
mlir::OpBuilder::InsertPoint insPt = builder.saveInsertionPoint();
160+
if (lastPrivIP && lastPrivIP->isSet())
161+
builder.restoreInsertionPoint(*lastPrivIP);
162+
else
163+
builder.setInsertionPointAfter(sb.getAddr().getDefiningOp());
164+
165+
hlfir::Entity dst{hsb.getAddr()};
166+
hlfir::Entity src{sb.getAddr()};
167+
builder.create<hlfir::AssignOp>(
168+
loc, src, dst, /*isWholeAllocatableAssignment=*/allocatable,
169+
/*keepLhsLengthInAllocatableAssignment=*/false, /*temporary_lhs=*/true);
170+
171+
if (lastPrivIP && lastPrivIP->isSet() &&
172+
sym->test(Fortran::semantics::Symbol::Flag::OmpLastPrivate)) {
173+
builder.restoreInsertionPoint(insPt);
174+
}
175+
}
132176
}
133177

134178
void DataSharingProcessor::collectOmpObjectListSymbol(

‎flang/test/Lower/OpenMP/lastprivate-allocatable.f90

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
! RUN: %flang_fc1 -emit-hlfir -o - -fopenmp %s | FileCheck %s
2+
! RUN: bbc -emit-hlfir -o - -fopenmp %s | FileCheck %s
3+
4+
program lastprivate_allocatable
5+
integer, allocatable :: a
6+
integer :: i
7+
! a is unallocated here
8+
!$omp parallel do lastprivate(a)
9+
do i=1,1
10+
a = 42
11+
enddo
12+
!$omp end parallel do
13+
! a should be allocated here
14+
end program
15+
16+
! CHECK-LABEL: func.func @_QQmain()
17+
! CHECK: %[[VAL_0:.*]] = fir.alloca !fir.box<!fir.heap<i32>> {bindc_name = "a", uniq_name = "_QFEa"}
18+
! CHECK: %[[VAL_1:.*]] = fir.zero_bits !fir.heap<i32>
19+
! CHECK: %[[VAL_2:.*]] = fir.embox %[[VAL_1]] : (!fir.heap<i32>) -> !fir.box<!fir.heap<i32>>
20+
! CHECK: fir.store %[[VAL_2]] to %[[VAL_0]] : !fir.ref<!fir.box<!fir.heap<i32>>>
21+
! CHECK: %[[VAL_3:.*]]:2 = hlfir.declare %[[VAL_0]] {fortran_attrs = {{.*}}<allocatable>, uniq_name = "_QFEa"} : (!fir.ref<!fir.box<!fir.heap<i32>>>) -> (!fir.ref<!fir.box<!fir.heap<i32>>>, !fir.ref<!fir.box<!fir.heap<i32>>>)
22+
! CHECK: omp.parallel {
23+
! create original copy of private variable
24+
! CHECK: %[[VAL_16:.*]]:2 = hlfir.declare %{{.*}} {fortran_attrs = {{.*}}<allocatable>, uniq_name = "_QFEa"} : (!fir.ref<!fir.box<!fir.heap<i32>>>) -> (!fir.ref<!fir.box<!fir.heap<i32>>>, !fir.ref<!fir.box<!fir.heap<i32>>>)
25+
! CHECK: %[[VAL_17:.*]] = fir.alloca i32 {bindc_name = "i", pinned, uniq_name = "_QFEi"}
26+
! CHECK: %[[VAL_18:.*]]:2 = hlfir.declare %[[VAL_17]] {uniq_name = "_QFEi"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
27+
! CHECK: omp.wsloop {
28+
! CHECK: omp.loop_nest
29+
! [...]
30+
! if this is the last iteration
31+
! CHECK: fir.if %{{.*}} {
32+
! store loop IV
33+
! CHECK: fir.store %{{.*}} to %[[VAL_18]]#1 : !fir.ref<i32>
34+
! assign private variable to original copy: realloc and temporary_lhs
35+
! CHECK: hlfir.assign %[[VAL_16]]#0 to %[[VAL_3]]#0 realloc temporary_lhs : !fir.ref<!fir.box<!fir.heap<i32>>>, !fir.ref<!fir.box<!fir.heap<i32>>>
36+
! CHECK-NEXT: }
37+
! CHECK-NEXT: omp.yield
38+
! CHECK-NEXT: }
39+
! CHECK-NEXT: omp.terminator
40+
! CHECK-NEXT: }
41+
! [no deallocation here]
42+
! CHECK-NEXT: omp.terminator
43+
! CHECK-NEXT: }
44+

0 commit comments

Comments
 (0)