Skip to content

Commit 1429f12

Browse files
ergawyergawy
committed
Reapply "[flang][OpenMP] Try to unify induction var privatization for OMP regions. (llvm#91116)"
This reapplies the referenced PR after finding a fix for the previously failing test suite tests.
1 parent f81da75 commit 1429f12

File tree

61 files changed

+388
-216
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

61 files changed

+388
-216
lines changed

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -131,7 +131,8 @@ class AbstractConverter {
131131

132132
/// For a given symbol, check if it is present in the inner-most
133133
/// level of the symbol map.
134-
virtual bool isPresentShallowLookup(Fortran::semantics::Symbol &sym) = 0;
134+
virtual bool
135+
isPresentShallowLookup(const Fortran::semantics::Symbol &sym) = 0;
135136

136137
/// Collect the set of symbols with \p flag in \p eval
137138
/// region if \p collectSymbols is true. Otherwise, collect the

‎flang/lib/Lower/Bridge.cpp

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -602,7 +602,8 @@ class FirConverter : public Fortran::lower::AbstractConverter {
602602
return typeConstructionStack;
603603
}
604604

605-
bool isPresentShallowLookup(Fortran::semantics::Symbol &sym) override final {
605+
bool
606+
isPresentShallowLookup(const Fortran::semantics::Symbol &sym) override final {
606607
return bool(shallowLookupSymbol(sym));
607608
}
608609

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

Lines changed: 73 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -29,9 +29,10 @@ void DataSharingProcessor::processStep1(
2929
collectSymbolsForPrivatization();
3030
collectDefaultSymbols();
3131
collectImplicitSymbols();
32+
collectPreDeterminedSymbols();
33+
3234
privatize(clauseOps, privateSyms);
33-
defaultPrivatize(clauseOps, privateSyms);
34-
implicitPrivatize(clauseOps, privateSyms);
35+
3536
insertBarrier();
3637
}
3738

@@ -57,7 +58,7 @@ void DataSharingProcessor::processStep2(mlir::Operation *op, bool isLoop) {
5758
}
5859

5960
void DataSharingProcessor::insertDeallocs() {
60-
for (const semantics::Symbol *sym : privatizedSymbols)
61+
for (const semantics::Symbol *sym : allPrivatizedSymbols)
6162
if (semantics::IsAllocatable(sym->GetUltimate())) {
6263
if (!useDelayedPrivatization) {
6364
converter.createHostAssociateVarCloneDealloc(*sym);
@@ -92,10 +93,6 @@ void DataSharingProcessor::insertDeallocs() {
9293
}
9394

9495
void DataSharingProcessor::cloneSymbol(const semantics::Symbol *sym) {
95-
// Privatization for symbols which are pre-determined (like loop index
96-
// variables) happen separately, for everything else privatize here.
97-
if (sym->test(semantics::Symbol::Flag::OmpPreDetermined))
98-
return;
9996
bool success = converter.createHostAssociateVarClone(*sym);
10097
(void)success;
10198
assert(success && "Privatization failed due to existing binding");
@@ -126,20 +123,24 @@ void DataSharingProcessor::collectSymbolsForPrivatization() {
126123
for (const omp::Clause &clause : clauses) {
127124
if (const auto &privateClause =
128125
std::get_if<omp::clause::Private>(&clause.u)) {
129-
collectOmpObjectListSymbol(privateClause->v, privatizedSymbols);
126+
collectOmpObjectListSymbol(privateClause->v, explicitlyPrivatizedSymbols);
130127
} else if (const auto &firstPrivateClause =
131128
std::get_if<omp::clause::Firstprivate>(&clause.u)) {
132-
collectOmpObjectListSymbol(firstPrivateClause->v, privatizedSymbols);
129+
collectOmpObjectListSymbol(firstPrivateClause->v,
130+
explicitlyPrivatizedSymbols);
133131
} else if (const auto &lastPrivateClause =
134132
std::get_if<omp::clause::Lastprivate>(&clause.u)) {
135133
const ObjectList &objects = std::get<ObjectList>(lastPrivateClause->t);
136-
collectOmpObjectListSymbol(objects, privatizedSymbols);
134+
collectOmpObjectListSymbol(objects, explicitlyPrivatizedSymbols);
137135
hasLastPrivateOp = true;
138136
} else if (std::get_if<omp::clause::Collapse>(&clause.u)) {
139137
hasCollapse = true;
140138
}
141139
}
142140

141+
for (auto *sym : explicitlyPrivatizedSymbols)
142+
allPrivatizedSymbols.insert(sym);
143+
143144
if (hasCollapse && hasLastPrivateOp)
144145
TODO(converter.getCurrentLocation(), "Collapse clause with lastprivate");
145146
}
@@ -149,7 +150,7 @@ bool DataSharingProcessor::needBarrier() {
149150
// initialization of firstprivate variables and post-update of lastprivate
150151
// variables.
151152
// Emit implicit barrier for linear clause. Maybe on somewhere else.
152-
for (const semantics::Symbol *sym : privatizedSymbols) {
153+
for (const semantics::Symbol *sym : allPrivatizedSymbols) {
153154
if (sym->test(semantics::Symbol::Flag::OmpFirstPrivate) &&
154155
sym->test(semantics::Symbol::Flag::OmpLastPrivate))
155156
return true;
@@ -283,10 +284,40 @@ void DataSharingProcessor::collectSymbolsInNestedRegions(
283284
if (nestedEval.isConstruct())
284285
// Recursively look for OpenMP constructs within `nestedEval`'s region
285286
collectSymbolsInNestedRegions(nestedEval, flag, symbolsInNestedRegions);
286-
else
287-
converter.collectSymbolSet(nestedEval, symbolsInNestedRegions, flag,
288-
/*collectSymbols=*/true,
289-
/*collectHostAssociatedSymbols=*/false);
287+
else {
288+
bool isOrderedConstruct = [&]() {
289+
if (auto *ompConstruct =
290+
nestedEval.getIf<parser::OpenMPConstruct>()) {
291+
if (auto *ompBlockConstruct =
292+
std::get_if<parser::OpenMPBlockConstruct>(
293+
&ompConstruct->u)) {
294+
const auto &beginBlockDirective =
295+
std::get<parser::OmpBeginBlockDirective>(
296+
ompBlockConstruct->t);
297+
const auto origDirective =
298+
std::get<parser::OmpBlockDirective>(beginBlockDirective.t).v;
299+
300+
return origDirective == llvm::omp::Directive::OMPD_ordered;
301+
}
302+
}
303+
304+
return false;
305+
}();
306+
307+
bool isCriticalConstruct = [&]() {
308+
if (auto *ompConstruct =
309+
nestedEval.getIf<parser::OpenMPConstruct>()) {
310+
return std::get_if<parser::OpenMPCriticalConstruct>(
311+
&ompConstruct->u) != nullptr;
312+
}
313+
return false;
314+
}();
315+
316+
if (!isOrderedConstruct && !isCriticalConstruct)
317+
converter.collectSymbolSet(nestedEval, symbolsInNestedRegions, flag,
318+
/*collectSymbols=*/true,
319+
/*collectHostAssociatedSymbols=*/false);
320+
}
290321
}
291322
}
292323
}
@@ -322,24 +353,39 @@ void DataSharingProcessor::collectSymbols(
322353
converter.collectSymbolSet(eval, allSymbols, flag,
323354
/*collectSymbols=*/true,
324355
/*collectHostAssociatedSymbols=*/true);
356+
325357
llvm::SetVector<const semantics::Symbol *> symbolsInNestedRegions;
326358
collectSymbolsInNestedRegions(eval, flag, symbolsInNestedRegions);
327359
// Filter-out symbols that must not be privatized.
328360
bool collectImplicit = flag == semantics::Symbol::Flag::OmpImplicit;
361+
bool collectPreDetermined = flag == semantics::Symbol::Flag::OmpPreDetermined;
362+
329363
auto isPrivatizable = [](const semantics::Symbol &sym) -> bool {
330364
return !semantics::IsProcedure(sym) &&
331365
!sym.GetUltimate().has<semantics::DerivedTypeDetails>() &&
332366
!sym.GetUltimate().has<semantics::NamelistDetails>() &&
333367
!semantics::IsImpliedDoIndex(sym.GetUltimate());
334368
};
369+
370+
auto shouldCollectSymbol = [&](const semantics::Symbol *sym) {
371+
if (collectImplicit)
372+
return sym->test(semantics::Symbol::Flag::OmpImplicit);
373+
374+
if (collectPreDetermined)
375+
return sym->test(semantics::Symbol::Flag::OmpPreDetermined);
376+
377+
return !sym->test(semantics::Symbol::Flag::OmpImplicit) &&
378+
!sym->test(semantics::Symbol::Flag::OmpPreDetermined);
379+
};
380+
335381
for (const auto *sym : allSymbols) {
336382
assert(curScope && "couldn't find current scope");
337383
if (isPrivatizable(*sym) && !symbolsInNestedRegions.contains(sym) &&
338-
!privatizedSymbols.contains(sym) &&
339-
!sym->test(semantics::Symbol::Flag::OmpPreDetermined) &&
340-
(collectImplicit || !sym->test(semantics::Symbol::Flag::OmpImplicit)) &&
341-
clauseScopes.contains(&sym->owner()))
384+
!explicitlyPrivatizedSymbols.contains(sym) &&
385+
shouldCollectSymbol(sym) && clauseScopes.contains(&sym->owner())) {
386+
allPrivatizedSymbols.insert(sym);
342387
symbols.insert(sym);
388+
}
343389
}
344390
}
345391

@@ -363,10 +409,16 @@ void DataSharingProcessor::collectImplicitSymbols() {
363409
collectSymbols(semantics::Symbol::Flag::OmpImplicit, implicitSymbols);
364410
}
365411

412+
void DataSharingProcessor::collectPreDeterminedSymbols() {
413+
if (shouldCollectPreDeterminedSymbols)
414+
collectSymbols(semantics::Symbol::Flag::OmpPreDetermined,
415+
preDeterminedSymbols);
416+
}
417+
366418
void DataSharingProcessor::privatize(
367419
mlir::omp::PrivateClauseOps *clauseOps,
368420
llvm::SmallVectorImpl<const semantics::Symbol *> *privateSyms) {
369-
for (const semantics::Symbol *sym : privatizedSymbols) {
421+
for (const semantics::Symbol *sym : allPrivatizedSymbols) {
370422
if (const auto *commonDet =
371423
sym->detailsIf<semantics::CommonBlockDetails>()) {
372424
for (const auto &mem : commonDet->objects())
@@ -378,7 +430,7 @@ void DataSharingProcessor::privatize(
378430

379431
void DataSharingProcessor::copyLastPrivatize(mlir::Operation *op) {
380432
insertLastPrivateCompare(op);
381-
for (const semantics::Symbol *sym : privatizedSymbols)
433+
for (const semantics::Symbol *sym : allPrivatizedSymbols)
382434
if (const auto *commonDet =
383435
sym->detailsIf<semantics::CommonBlockDetails>()) {
384436
for (const auto &mem : commonDet->objects()) {
@@ -389,20 +441,6 @@ void DataSharingProcessor::copyLastPrivatize(mlir::Operation *op) {
389441
}
390442
}
391443

392-
void DataSharingProcessor::defaultPrivatize(
393-
mlir::omp::PrivateClauseOps *clauseOps,
394-
llvm::SmallVectorImpl<const semantics::Symbol *> *privateSyms) {
395-
for (const semantics::Symbol *sym : defaultSymbols)
396-
doPrivatize(sym, clauseOps, privateSyms);
397-
}
398-
399-
void DataSharingProcessor::implicitPrivatize(
400-
mlir::omp::PrivateClauseOps *clauseOps,
401-
llvm::SmallVectorImpl<const semantics::Symbol *> *privateSyms) {
402-
for (const semantics::Symbol *sym : implicitSymbols)
403-
doPrivatize(sym, clauseOps, privateSyms);
404-
}
405-
406444
void DataSharingProcessor::doPrivatize(
407445
const semantics::Symbol *sym, mlir::omp::PrivateClauseOps *clauseOps,
408446
llvm::SmallVectorImpl<const semantics::Symbol *> *privateSyms) {

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

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,16 +37,20 @@ class DataSharingProcessor {
3737
mlir::OpBuilder::InsertPoint insPt;
3838
mlir::Value loopIV;
3939
// Symbols in private, firstprivate, and/or lastprivate clauses.
40-
llvm::SetVector<const semantics::Symbol *> privatizedSymbols;
40+
llvm::SetVector<const semantics::Symbol *> explicitlyPrivatizedSymbols;
4141
llvm::SetVector<const semantics::Symbol *> defaultSymbols;
4242
llvm::SetVector<const semantics::Symbol *> implicitSymbols;
43+
llvm::SetVector<const semantics::Symbol *> preDeterminedSymbols;
44+
llvm::SetVector<const semantics::Symbol *> allPrivatizedSymbols;
45+
4346
llvm::DenseMap<const semantics::Symbol *, mlir::omp::PrivateClauseOp>
4447
symToPrivatizer;
4548
lower::AbstractConverter &converter;
4649
semantics::SemanticsContext &semaCtx;
4750
fir::FirOpBuilder &firOpBuilder;
4851
omp::List<omp::Clause> clauses;
4952
lower::pft::Evaluation &eval;
53+
bool shouldCollectPreDeterminedSymbols;
5054
bool useDelayedPrivatization;
5155
lower::SymMap *symTable;
5256

@@ -63,6 +67,7 @@ class DataSharingProcessor {
6367
void insertBarrier();
6468
void collectDefaultSymbols();
6569
void collectImplicitSymbols();
70+
void collectPreDeterminedSymbols();
6671
void privatize(mlir::omp::PrivateClauseOps *clauseOps,
6772
llvm::SmallVectorImpl<const semantics::Symbol *> *privateSyms);
6873
void defaultPrivatize(
@@ -90,10 +95,12 @@ class DataSharingProcessor {
9095
semantics::SemanticsContext &semaCtx,
9196
const List<Clause> &clauses,
9297
lower::pft::Evaluation &eval,
98+
bool shouldCollectPreDeterminedSymbols,
9399
bool useDelayedPrivatization = false,
94100
lower::SymMap *symTable = nullptr)
95101
: hasLastPrivateOp(false), converter(converter), semaCtx(semaCtx),
96102
firOpBuilder(converter.getFirOpBuilder()), clauses(clauses), eval(eval),
103+
shouldCollectPreDeterminedSymbols(shouldCollectPreDeterminedSymbols),
97104
useDelayedPrivatization(useDelayedPrivatization), symTable(symTable) {}
98105

99106
// Privatisation is split into two steps.

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -123,4 +123,9 @@ ConstructQueue buildConstructQueue(
123123

124124
return constructs;
125125
}
126+
127+
bool isLastItemInQueue(ConstructQueue::iterator item,
128+
const ConstructQueue &queue) {
129+
return std::next(item) == queue.end();
130+
}
126131
} // namespace Fortran::lower::omp

‎flang/lib/Lower/OpenMP/Decomposer.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,9 @@ ConstructQueue buildConstructQueue(mlir::ModuleOp modOp,
4646
const parser::CharBlock &source,
4747
llvm::omp::Directive compound,
4848
const List<Clause> &clauses);
49+
50+
bool isLastItemInQueue(ConstructQueue::iterator item,
51+
const ConstructQueue &queue);
4952
} // namespace Fortran::lower::omp
5053

5154
#endif // FORTRAN_LOWER_OPENMP_DECOMPOSER_H

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

Lines changed: 17 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -215,12 +215,10 @@ createAndSetPrivatizedLoopVar(lower::AbstractConverter &converter,
215215
firOpBuilder.setInsertionPointToStart(firOpBuilder.getAllocaBlock());
216216

217217
mlir::Type tempTy = converter.genType(*sym);
218-
mlir::Value temp = firOpBuilder.create<fir::AllocaOp>(
219-
loc, tempTy, /*pinned=*/true, /*lengthParams=*/mlir::ValueRange{},
220-
/*shapeParams*/ mlir::ValueRange{},
221-
llvm::ArrayRef<mlir::NamedAttribute>{
222-
fir::getAdaptToByRefAttr(firOpBuilder)});
223-
converter.bindSymbol(*sym, temp);
218+
219+
assert(converter.isPresentShallowLookup(*sym) &&
220+
"Expected symbol to be in symbol table.");
221+
224222
firOpBuilder.restoreInsertionPoint(insPt);
225223
mlir::Value cvtVal = firOpBuilder.createConvert(loc, tempTy, indexVal);
226224
mlir::Operation *storeOp = firOpBuilder.create<fir::StoreOp>(
@@ -580,7 +578,8 @@ static void createBodyOfOp(mlir::Operation &op, const OpWithBodyGenInfo &info,
580578
std::optional<DataSharingProcessor> tempDsp;
581579
if (privatize) {
582580
if (!info.dsp) {
583-
tempDsp.emplace(info.converter, info.semaCtx, *info.clauses, info.eval);
581+
tempDsp.emplace(info.converter, info.semaCtx, *info.clauses, info.eval,
582+
Fortran::lower::omp::isLastItemInQueue(item, queue));
584583
tempDsp->processStep1();
585584
}
586585
}
@@ -1316,6 +1315,7 @@ genParallelOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
13161315

13171316
bool privatize = !outerCombined;
13181317
DataSharingProcessor dsp(converter, semaCtx, item->clauses, eval,
1318+
lower::omp::isLastItemInQueue(item, queue),
13191319
/*useDelayedPrivatization=*/true, &symTable);
13201320

13211321
if (privatize)
@@ -1388,7 +1388,8 @@ genSectionsOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
13881388

13891389
// Insert privatizations before SECTIONS
13901390
symTable.pushScope();
1391-
DataSharingProcessor dsp(converter, semaCtx, item->clauses, eval);
1391+
DataSharingProcessor dsp(converter, semaCtx, item->clauses, eval,
1392+
lower::omp::isLastItemInQueue(item, queue));
13921393
dsp.processStep1();
13931394

13941395
List<Clause> nonDsaClauses;
@@ -1458,7 +1459,9 @@ genSimdOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
14581459
mlir::Location loc, const ConstructQueue &queue,
14591460
ConstructQueue::iterator item) {
14601461
fir::FirOpBuilder &firOpBuilder = converter.getFirOpBuilder();
1461-
DataSharingProcessor dsp(converter, semaCtx, item->clauses, eval);
1462+
symTable.pushScope();
1463+
DataSharingProcessor dsp(converter, semaCtx, item->clauses, eval,
1464+
lower::omp::isLastItemInQueue(item, queue));
14621465
dsp.processStep1();
14631466

14641467
lower::StatementContext stmtCtx;
@@ -1496,6 +1499,7 @@ genSimdOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
14961499
.setGenRegionEntryCb(ivCallback),
14971500
queue, item);
14981501

1502+
symTable.popScope();
14991503
return simdOp;
15001504
}
15011505

@@ -1764,7 +1768,9 @@ genWsloopOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
17641768
mlir::Location loc, const ConstructQueue &queue,
17651769
ConstructQueue::iterator item) {
17661770
fir::FirOpBuilder &firOpBuilder = converter.getFirOpBuilder();
1767-
DataSharingProcessor dsp(converter, semaCtx, item->clauses, eval);
1771+
symTable.pushScope();
1772+
DataSharingProcessor dsp(converter, semaCtx, item->clauses, eval,
1773+
lower::omp::isLastItemInQueue(item, queue));
17681774
dsp.processStep1();
17691775

17701776
lower::StatementContext stmtCtx;
@@ -1807,6 +1813,7 @@ genWsloopOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
18071813
.setReductions(&reductionSyms, &reductionTypes)
18081814
.setGenRegionEntryCb(ivCallback),
18091815
queue, item);
1816+
symTable.popScope();
18101817
return wsloopOp;
18111818
}
18121819

‎flang/test/Lower/OpenMP/Todo/omp-default-clause-inner-loop.f90

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
! CHECK: omp.parallel {
99
! EXPECTED: %[[PRIVATE_Y:.*]] = fir.alloca i32 {bindc_name = "y", pinned, uniq_name = "_QFEy"}
1010
! EXPECTED: %[[PRIVATE_Z:.*]] = fir.alloca i32 {bindc_name = "z", pinned, uniq_name = "_QFEz"}
11-
! CHECK: %[[TEMP:.*]] = fir.alloca i32 {adapt.valuebyref, pinned}
11+
! CHECK: %[[TEMP:.*]] = fir.alloca i32 {bindc_name = "x", pinned, {{.*}}}
1212
! CHECK: %[[const_1:.*]] = arith.constant 1 : i32
1313
! CHECK: %[[const_2:.*]] = arith.constant 10 : i32
1414
! CHECK: %[[const_3:.*]] = arith.constant 1 : i32

0 commit comments

Comments
 (0)