Skip to content

Commit 25f29ee

Browse files
ergawyergawy
authored
[flang][OpenMP] Update all lastprivate symbols, not just in clauses (llvm#125628)
Fixes a bug in updating `lastprivate` variables. Previously, we only iterated over the symbols collected from `lastprivate` clauses. This meants that for pre-determined symbols, we did not implement the update correctly (e.g. for loop iteration variables of `simd` constructs).
1 parent 46b1543 commit 25f29ee

File tree

2 files changed

+139
-73
lines changed

2 files changed

+139
-73
lines changed

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

+79-73
Original file line numberDiff line numberDiff line change
@@ -207,6 +207,9 @@ void DataSharingProcessor::collectSymbolsForPrivatization() {
207207
}
208208
}
209209

210+
// TODO For common blocks, add the underlying objects within the block. Doing
211+
// so, we won't need to explicitely handle block objects (or forget to do
212+
// so).
210213
for (auto *sym : explicitlyPrivatizedSymbols)
211214
allPrivatizedSymbols.insert(sym);
212215
}
@@ -235,82 +238,85 @@ void DataSharingProcessor::insertLastPrivateCompare(mlir::Operation *op) {
235238
if (auto wrapper = mlir::dyn_cast<mlir::omp::LoopWrapperInterface>(op))
236239
loopOp = mlir::cast<mlir::omp::LoopNestOp>(wrapper.getWrappedLoop());
237240

238-
bool cmpCreated = false;
239241
mlir::OpBuilder::InsertionGuard guard(firOpBuilder);
240-
for (const omp::Clause &clause : clauses) {
241-
if (clause.id != llvm::omp::OMPC_lastprivate)
242-
continue;
243-
if (mlir::isa<mlir::omp::WsloopOp>(op) ||
244-
mlir::isa<mlir::omp::SimdOp>(op)) {
245-
// Update the original variable just before exiting the worksharing
246-
// loop. Conversion as follows:
247-
//
248-
// omp.wsloop / omp.simd { omp.wsloop / omp.simd {
249-
// omp.loop_nest { omp.loop_nest {
250-
// ... ...
251-
// store ===> store
252-
// omp.yield %v = arith.addi %iv, %step
253-
// } %cmp = %step < 0 ? %v < %ub : %v > %ub
254-
// } fir.if %cmp {
255-
// fir.store %v to %loopIV
256-
// ^%lpv_update_blk:
257-
// }
258-
// omp.yield
259-
// }
260-
// }
261-
262-
// Only generate the compare once in presence of multiple LastPrivate
263-
// clauses.
264-
if (cmpCreated)
265-
continue;
266-
cmpCreated = true;
267-
268-
mlir::Location loc = loopOp.getLoc();
269-
mlir::Operation *lastOper = loopOp.getRegion().back().getTerminator();
270-
firOpBuilder.setInsertionPoint(lastOper);
271-
272-
mlir::Value cmpOp;
273-
llvm::SmallVector<mlir::Value> vs;
274-
vs.reserve(loopOp.getIVs().size());
275-
for (auto [iv, ub, step] :
276-
llvm::zip_equal(loopOp.getIVs(), loopOp.getLoopUpperBounds(),
277-
loopOp.getLoopSteps())) {
278-
// v = iv + step
279-
// cmp = step < 0 ? v < ub : v > ub
280-
mlir::Value v = firOpBuilder.create<mlir::arith::AddIOp>(loc, iv, step);
281-
vs.push_back(v);
282-
mlir::Value zero =
283-
firOpBuilder.createIntegerConstant(loc, step.getType(), 0);
284-
mlir::Value negativeStep = firOpBuilder.create<mlir::arith::CmpIOp>(
285-
loc, mlir::arith::CmpIPredicate::slt, step, zero);
286-
mlir::Value vLT = firOpBuilder.create<mlir::arith::CmpIOp>(
287-
loc, mlir::arith::CmpIPredicate::slt, v, ub);
288-
mlir::Value vGT = firOpBuilder.create<mlir::arith::CmpIOp>(
289-
loc, mlir::arith::CmpIPredicate::sgt, v, ub);
290-
mlir::Value icmpOp = firOpBuilder.create<mlir::arith::SelectOp>(
291-
loc, negativeStep, vLT, vGT);
292-
293-
if (cmpOp) {
294-
cmpOp = firOpBuilder.create<mlir::arith::AndIOp>(loc, cmpOp, icmpOp);
295-
} else {
296-
cmpOp = icmpOp;
297-
}
298-
}
242+
bool hasLastPrivate = [&]() {
243+
for (const semantics::Symbol *sym : allPrivatizedSymbols) {
244+
if (const auto *commonDet =
245+
sym->detailsIf<semantics::CommonBlockDetails>()) {
246+
for (const auto &mem : commonDet->objects())
247+
if (mem->test(semantics::Symbol::Flag::OmpLastPrivate))
248+
return true;
249+
} else if (sym->test(semantics::Symbol::Flag::OmpLastPrivate))
250+
return true;
251+
}
299252

300-
auto ifOp = firOpBuilder.create<fir::IfOp>(loc, cmpOp, /*else*/ false);
301-
firOpBuilder.setInsertionPointToStart(&ifOp.getThenRegion().front());
302-
for (auto [v, loopIV] : llvm::zip_equal(vs, loopIVs)) {
303-
assert(loopIV && "loopIV was not set");
304-
firOpBuilder.createStoreWithConvert(loc, v, loopIV);
305-
}
306-
lastPrivIP = firOpBuilder.saveInsertionPoint();
307-
} else if (mlir::isa<mlir::omp::SectionsOp>(op)) {
308-
// Already handled by genOMP()
309-
} else {
310-
TODO(converter.getCurrentLocation(),
311-
"lastprivate clause in constructs other than "
312-
"simd/worksharing-loop");
253+
return false;
254+
}();
255+
256+
if (!hasLastPrivate)
257+
return;
258+
259+
if (mlir::isa<mlir::omp::WsloopOp>(op) || mlir::isa<mlir::omp::SimdOp>(op)) {
260+
// Update the original variable just before exiting the worksharing
261+
// loop. Conversion as follows:
262+
//
263+
// omp.wsloop / omp.simd { omp.wsloop / omp.simd {
264+
// omp.loop_nest { omp.loop_nest {
265+
// ... ...
266+
// store ===> store
267+
// omp.yield %v = arith.addi %iv, %step
268+
// } %cmp = %step < 0 ? %v < %ub : %v > %ub
269+
// } fir.if %cmp {
270+
// fir.store %v to %loopIV
271+
// ^%lpv_update_blk:
272+
// }
273+
// omp.yield
274+
// }
275+
// }
276+
mlir::Location loc = loopOp.getLoc();
277+
mlir::Operation *lastOper = loopOp.getRegion().back().getTerminator();
278+
firOpBuilder.setInsertionPoint(lastOper);
279+
280+
mlir::Value cmpOp;
281+
llvm::SmallVector<mlir::Value> vs;
282+
vs.reserve(loopOp.getIVs().size());
283+
for (auto [iv, ub, step] :
284+
llvm::zip_equal(loopOp.getIVs(), loopOp.getLoopUpperBounds(),
285+
loopOp.getLoopSteps())) {
286+
// v = iv + step
287+
// cmp = step < 0 ? v < ub : v > ub
288+
mlir::Value v = firOpBuilder.create<mlir::arith::AddIOp>(loc, iv, step);
289+
vs.push_back(v);
290+
mlir::Value zero =
291+
firOpBuilder.createIntegerConstant(loc, step.getType(), 0);
292+
mlir::Value negativeStep = firOpBuilder.create<mlir::arith::CmpIOp>(
293+
loc, mlir::arith::CmpIPredicate::slt, step, zero);
294+
mlir::Value vLT = firOpBuilder.create<mlir::arith::CmpIOp>(
295+
loc, mlir::arith::CmpIPredicate::slt, v, ub);
296+
mlir::Value vGT = firOpBuilder.create<mlir::arith::CmpIOp>(
297+
loc, mlir::arith::CmpIPredicate::sgt, v, ub);
298+
mlir::Value icmpOp = firOpBuilder.create<mlir::arith::SelectOp>(
299+
loc, negativeStep, vLT, vGT);
300+
301+
if (cmpOp)
302+
cmpOp = firOpBuilder.create<mlir::arith::AndIOp>(loc, cmpOp, icmpOp);
303+
else
304+
cmpOp = icmpOp;
313305
}
306+
307+
auto ifOp = firOpBuilder.create<fir::IfOp>(loc, cmpOp, /*else*/ false);
308+
firOpBuilder.setInsertionPointToStart(&ifOp.getThenRegion().front());
309+
for (auto [v, loopIV] : llvm::zip_equal(vs, loopIVs)) {
310+
assert(loopIV && "loopIV was not set");
311+
firOpBuilder.createStoreWithConvert(loc, v, loopIV);
312+
}
313+
lastPrivIP = firOpBuilder.saveInsertionPoint();
314+
} else if (mlir::isa<mlir::omp::SectionsOp>(op)) {
315+
// Already handled by genOMP()
316+
} else {
317+
TODO(converter.getCurrentLocation(),
318+
"lastprivate clause in constructs other than "
319+
"simd/worksharing-loop");
314320
}
315321
}
316322

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

+60
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
! RUN: %flang_fc1 -emit-hlfir -fopenmp -o - %s 2>&1 | FileCheck %s
2+
3+
subroutine simd_ivs
4+
implicit none
5+
integer :: ido1 = 1
6+
integer :: ido2 = 2
7+
integer :: ido3 = 3
8+
integer :: ido4 = 4
9+
10+
!$omp parallel
11+
!$omp simd collapse(3)
12+
do ido1 = 1, 10
13+
do ido2 = 1, 10
14+
do ido3 = 1, 10
15+
do ido4 = 1, 10
16+
end do
17+
end do
18+
end do
19+
end do
20+
!$omp end simd
21+
!$omp end parallel
22+
end subroutine
23+
24+
! CHECK: func.func @_QPsimd_ivs() {
25+
! CHECK: %[[IDO1_HOST_DECL:.*]]:2 = hlfir.declare %{{.*}} {uniq_name = "{{.*}}Eido1"}
26+
! CHECK: %[[IDO2_HOST_DECL:.*]]:2 = hlfir.declare %{{.*}} {uniq_name = "{{.*}}Eido2"}
27+
! CHECK: %[[IDO3_HOST_DECL:.*]]:2 = hlfir.declare %{{.*}} {uniq_name = "{{.*}}Eido3"}
28+
29+
! CHECK: omp.parallel {
30+
! CHECK: omp.simd private(
31+
! CHECK-SAME: @{{.*}}do1_private{{.*}} %[[IDO1_HOST_DECL]]#0 -> %[[IDO1_PRIV_ARG:[^[:space:]]*]],
32+
! CHECK-SAME: @{{.*}}do2_private{{.*}} %[[IDO2_HOST_DECL]]#0 -> %[[IDO2_PRIV_ARG:[^[:space:]]*]],
33+
! CHECK-SAME: @{{.*}}do3_private{{.*}} %[[IDO3_HOST_DECL]]#0 -> %[[IDO3_PRIV_ARG:[^[:space:]]*]]
34+
! CHECK-SAME: : {{.*}}) {
35+
36+
! CHECK: omp.loop_nest (%[[IV1:.*]], %[[IV2:.*]], %[[IV3:.*]]) : {{.*}} {
37+
! CHECK: %[[IDO1_PRIV_DECL:.*]]:2 = hlfir.declare %[[IDO1_PRIV_ARG]] {uniq_name = "{{.*}}Eido1"}
38+
! CHECK: %[[IDO2_PRIV_DECL:.*]]:2 = hlfir.declare %[[IDO2_PRIV_ARG]] {uniq_name = "{{.*}}Eido2"}
39+
! CHECK: %[[IDO3_PRIV_DECL:.*]]:2 = hlfir.declare %[[IDO3_PRIV_ARG]] {uniq_name = "{{.*}}Eido3"}
40+
41+
! CHECK: fir.if %33 {
42+
! CHECK: fir.store %{{.*}} to %[[IDO1_PRIV_DECL]]#1
43+
! CHECK: fir.store %{{.*}} to %[[IDO2_PRIV_DECL]]#1
44+
! CHECK: fir.store %{{.*}} to %[[IDO3_PRIV_DECL]]#1
45+
! CHECK: %[[IDO1_VAL:.*]] = fir.load %[[IDO1_PRIV_DECL]]#0
46+
! CHECK: hlfir.assign %[[IDO1_VAL]] to %[[IDO1_HOST_DECL]]#0
47+
! CHECK: %[[IDO2_VAL:.*]] = fir.load %[[IDO2_PRIV_DECL]]#0
48+
! CHECK: hlfir.assign %[[IDO2_VAL]] to %[[IDO2_HOST_DECL]]#0
49+
! CHECK: %[[IDO3_VAL:.*]] = fir.load %[[IDO3_PRIV_DECL]]#0
50+
! CHECK: hlfir.assign %[[IDO3_VAL]] to %[[IDO3_HOST_DECL]]#0
51+
! CHECK: }
52+
! CHECK-NEXT: omp.yield
53+
! CHECK: }
54+
55+
! CHECK: }
56+
57+
! CHECK: omp.terminator
58+
! CHECK: }
59+
60+
! CHECK: }

0 commit comments

Comments
 (0)