Skip to content

Commit a9d0d45

Browse files
ronliebronlieb
authored
Merge pull request #500 from AMD-Lightning-Internal/amd/dev/rlieberm/revert-flang-lastpriv
Revert "[flang][OpenMP] Update all `lastprivate` symbols, not just in…
2 parents 8b47882 + 8c99c70 commit a9d0d45

File tree

3 files changed

+77
-139
lines changed

3 files changed

+77
-139
lines changed

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

Lines changed: 73 additions & 79 deletions
Original file line numberDiff line numberDiff line change
@@ -179,9 +179,6 @@ void DataSharingProcessor::collectSymbolsForPrivatization() {
179179
}
180180
}
181181

182-
// TODO For common blocks, add the underlying objects within the block. Doing
183-
// so, we won't need to explicitely handle block objects (or forget to do
184-
// so).
185182
for (auto *sym : explicitlyPrivatizedSymbols)
186183
allPrivatizedSymbols.insert(sym);
187184
}
@@ -209,85 +206,82 @@ void DataSharingProcessor::insertLastPrivateCompare(mlir::Operation *op) {
209206
if (auto wrapper = mlir::dyn_cast<mlir::omp::LoopWrapperInterface>(op))
210207
loopOp = mlir::cast<mlir::omp::LoopNestOp>(wrapper.getWrappedLoop());
211208

209+
bool cmpCreated = false;
212210
mlir::OpBuilder::InsertionGuard guard(firOpBuilder);
213-
bool hasLastPrivate = [&]() {
214-
for (const semantics::Symbol *sym : allPrivatizedSymbols) {
215-
if (const auto *commonDet =
216-
sym->detailsIf<semantics::CommonBlockDetails>()) {
217-
for (const auto &mem : commonDet->objects())
218-
if (mem->test(semantics::Symbol::Flag::OmpLastPrivate))
219-
return true;
220-
} else if (sym->test(semantics::Symbol::Flag::OmpLastPrivate))
221-
return true;
222-
}
223-
224-
return false;
225-
}();
226-
227-
if (!hasLastPrivate)
228-
return;
229-
230-
if (mlir::isa<mlir::omp::WsloopOp>(op) || mlir::isa<mlir::omp::SimdOp>(op)) {
231-
// Update the original variable just before exiting the worksharing
232-
// loop. Conversion as follows:
233-
//
234-
// omp.wsloop / omp.simd { omp.wsloop / omp.simd {
235-
// omp.loop_nest { omp.loop_nest {
236-
// ... ...
237-
// store ===> store
238-
// omp.yield %v = arith.addi %iv, %step
239-
// } %cmp = %step < 0 ? %v < %ub : %v > %ub
240-
// } fir.if %cmp {
241-
// fir.store %v to %loopIV
242-
// ^%lpv_update_blk:
243-
// }
244-
// omp.yield
245-
// }
246-
// }
247-
mlir::Location loc = loopOp.getLoc();
248-
mlir::Operation *lastOper = loopOp.getRegion().back().getTerminator();
249-
firOpBuilder.setInsertionPoint(lastOper);
250-
251-
mlir::Value cmpOp;
252-
llvm::SmallVector<mlir::Value> vs;
253-
vs.reserve(loopOp.getIVs().size());
254-
for (auto [iv, ub, step] :
255-
llvm::zip_equal(loopOp.getIVs(), loopOp.getLoopUpperBounds(),
256-
loopOp.getLoopSteps())) {
257-
// v = iv + step
258-
// cmp = step < 0 ? v < ub : v > ub
259-
mlir::Value v = firOpBuilder.create<mlir::arith::AddIOp>(loc, iv, step);
260-
vs.push_back(v);
261-
mlir::Value zero =
262-
firOpBuilder.createIntegerConstant(loc, step.getType(), 0);
263-
mlir::Value negativeStep = firOpBuilder.create<mlir::arith::CmpIOp>(
264-
loc, mlir::arith::CmpIPredicate::slt, step, zero);
265-
mlir::Value vLT = firOpBuilder.create<mlir::arith::CmpIOp>(
266-
loc, mlir::arith::CmpIPredicate::slt, v, ub);
267-
mlir::Value vGT = firOpBuilder.create<mlir::arith::CmpIOp>(
268-
loc, mlir::arith::CmpIPredicate::sgt, v, ub);
269-
mlir::Value icmpOp = firOpBuilder.create<mlir::arith::SelectOp>(
270-
loc, negativeStep, vLT, vGT);
271-
272-
if (cmpOp)
273-
cmpOp = firOpBuilder.create<mlir::arith::AndIOp>(loc, cmpOp, icmpOp);
274-
else
275-
cmpOp = icmpOp;
276-
}
211+
for (const omp::Clause &clause : clauses) {
212+
if (clause.id != llvm::omp::OMPC_lastprivate)
213+
continue;
214+
if (mlir::isa<mlir::omp::WsloopOp>(op) ||
215+
mlir::isa<mlir::omp::SimdOp>(op)) {
216+
// Update the original variable just before exiting the worksharing
217+
// loop. Conversion as follows:
218+
//
219+
// omp.wsloop / omp.simd { omp.wsloop / omp.simd {
220+
// omp.loop_nest { omp.loop_nest {
221+
// ... ...
222+
// store ===> store
223+
// omp.yield %v = arith.addi %iv, %step
224+
// } %cmp = %step < 0 ? %v < %ub : %v > %ub
225+
// } fir.if %cmp {
226+
// fir.store %v to %loopIV
227+
// ^%lpv_update_blk:
228+
// }
229+
// omp.yield
230+
// }
231+
// }
232+
233+
// Only generate the compare once in presence of multiple LastPrivate
234+
// clauses.
235+
if (cmpCreated)
236+
continue;
237+
cmpCreated = true;
238+
239+
mlir::Location loc = loopOp.getLoc();
240+
mlir::Operation *lastOper = loopOp.getRegion().back().getTerminator();
241+
firOpBuilder.setInsertionPoint(lastOper);
242+
243+
mlir::Value cmpOp;
244+
llvm::SmallVector<mlir::Value> vs;
245+
vs.reserve(loopOp.getIVs().size());
246+
for (auto [iv, ub, step] :
247+
llvm::zip_equal(loopOp.getIVs(), loopOp.getLoopUpperBounds(),
248+
loopOp.getLoopSteps())) {
249+
// v = iv + step
250+
// cmp = step < 0 ? v < ub : v > ub
251+
mlir::Value v = firOpBuilder.create<mlir::arith::AddIOp>(loc, iv, step);
252+
vs.push_back(v);
253+
mlir::Value zero =
254+
firOpBuilder.createIntegerConstant(loc, step.getType(), 0);
255+
mlir::Value negativeStep = firOpBuilder.create<mlir::arith::CmpIOp>(
256+
loc, mlir::arith::CmpIPredicate::slt, step, zero);
257+
mlir::Value vLT = firOpBuilder.create<mlir::arith::CmpIOp>(
258+
loc, mlir::arith::CmpIPredicate::slt, v, ub);
259+
mlir::Value vGT = firOpBuilder.create<mlir::arith::CmpIOp>(
260+
loc, mlir::arith::CmpIPredicate::sgt, v, ub);
261+
mlir::Value icmpOp = firOpBuilder.create<mlir::arith::SelectOp>(
262+
loc, negativeStep, vLT, vGT);
263+
264+
if (cmpOp) {
265+
cmpOp = firOpBuilder.create<mlir::arith::AndIOp>(loc, cmpOp, icmpOp);
266+
} else {
267+
cmpOp = icmpOp;
268+
}
269+
}
277270

278-
auto ifOp = firOpBuilder.create<fir::IfOp>(loc, cmpOp, /*else*/ false);
279-
firOpBuilder.setInsertionPointToStart(&ifOp.getThenRegion().front());
280-
for (auto [v, loopIV] : llvm::zip_equal(vs, loopIVs)) {
281-
assert(loopIV && "loopIV was not set");
282-
firOpBuilder.createStoreWithConvert(loc, v, loopIV);
271+
auto ifOp = firOpBuilder.create<fir::IfOp>(loc, cmpOp, /*else*/ false);
272+
firOpBuilder.setInsertionPointToStart(&ifOp.getThenRegion().front());
273+
for (auto [v, loopIV] : llvm::zip_equal(vs, loopIVs)) {
274+
assert(loopIV && "loopIV was not set");
275+
firOpBuilder.createStoreWithConvert(loc, v, loopIV);
276+
}
277+
lastPrivIP = firOpBuilder.saveInsertionPoint();
278+
} else if (mlir::isa<mlir::omp::SectionsOp>(op)) {
279+
// Already handled by genOMP()
280+
} else {
281+
TODO(converter.getCurrentLocation(),
282+
"lastprivate clause in constructs other than "
283+
"simd/worksharing-loop");
283284
}
284-
lastPrivIP = firOpBuilder.saveInsertionPoint();
285-
} else if (mlir::isa<mlir::omp::SectionsOp>(op)) {
286-
// Already handled by genOMP()
287-
} else {
288-
TODO(converter.getCurrentLocation(),
289-
"lastprivate clause in constructs other than "
290-
"simd/worksharing-loop");
291285
}
292286
}
293287

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

Lines changed: 0 additions & 60 deletions
This file was deleted.

‎revert_patches.txt‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,3 +35,7 @@ revert: needs flang-new merge help
3535
[mlir][OpenMP][flang] make private variable allocation implicit in omp.private (#124019)
3636
9ad4ebd82b60 [mlir][OpenMP][NFC] break out priv var init into hel
3737
---
38+
revert : breaks build of 5 spec-accel codes
39+
25f29ee377b1 - [flang][OpenMP] Update all `lastprivate` symbols, not just in clauses (#125628)
40+
Kareem, Ron
41+
---

0 commit comments

Comments
 (0)