Skip to content

Commit 85e1124

Browse files
omjavaidomjavaid
committed
Revert "[flang][OpenMP] Try to unify induction var privatization for OMP regions. (#91116)"
This reverts commit 2a97b50. It has broken LLVM testsuite on various bots https://lab.llvm.org/buildbot/#/builders/184/builds/12760 https://lab.llvm.org/buildbot/#/builders/197/builds/14376 https://lab.llvm.org/buildbot/#/builders/179/builds/10176
1 parent 4cebe5a commit 85e1124

File tree

61 files changed

+216
-388
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

+216
-388
lines changed

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

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -131,8 +131,7 @@ 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
135-
isPresentShallowLookup(const Fortran::semantics::Symbol &sym) = 0;
134+
virtual bool isPresentShallowLookup(Fortran::semantics::Symbol &sym) = 0;
136135

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

‎flang/lib/Lower/Bridge.cpp‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -601,8 +601,7 @@ class FirConverter : public Fortran::lower::AbstractConverter {
601601
return typeConstructionStack;
602602
}
603603

604-
bool
605-
isPresentShallowLookup(const Fortran::semantics::Symbol &sym) override final {
604+
bool isPresentShallowLookup(Fortran::semantics::Symbol &sym) override final {
606605
return bool(shallowLookupSymbol(sym));
607606
}
608607

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

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

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

6059
void DataSharingProcessor::insertDeallocs() {
61-
for (const semantics::Symbol *sym : allPrivatizedSymbols)
60+
for (const semantics::Symbol *sym : privatizedSymbols)
6261
if (semantics::IsAllocatable(sym->GetUltimate())) {
6362
if (!useDelayedPrivatization) {
6463
converter.createHostAssociateVarCloneDealloc(*sym);
@@ -93,6 +92,10 @@ void DataSharingProcessor::insertDeallocs() {
9392
}
9493

9594
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;
9699
bool success = converter.createHostAssociateVarClone(*sym);
97100
(void)success;
98101
assert(success && "Privatization failed due to existing binding");
@@ -123,24 +126,20 @@ void DataSharingProcessor::collectSymbolsForPrivatization() {
123126
for (const omp::Clause &clause : clauses) {
124127
if (const auto &privateClause =
125128
std::get_if<omp::clause::Private>(&clause.u)) {
126-
collectOmpObjectListSymbol(privateClause->v, explicitlyPrivatizedSymbols);
129+
collectOmpObjectListSymbol(privateClause->v, privatizedSymbols);
127130
} else if (const auto &firstPrivateClause =
128131
std::get_if<omp::clause::Firstprivate>(&clause.u)) {
129-
collectOmpObjectListSymbol(firstPrivateClause->v,
130-
explicitlyPrivatizedSymbols);
132+
collectOmpObjectListSymbol(firstPrivateClause->v, privatizedSymbols);
131133
} else if (const auto &lastPrivateClause =
132134
std::get_if<omp::clause::Lastprivate>(&clause.u)) {
133135
const ObjectList &objects = std::get<ObjectList>(lastPrivateClause->t);
134-
collectOmpObjectListSymbol(objects, explicitlyPrivatizedSymbols);
136+
collectOmpObjectListSymbol(objects, privatizedSymbols);
135137
hasLastPrivateOp = true;
136138
} else if (std::get_if<omp::clause::Collapse>(&clause.u)) {
137139
hasCollapse = true;
138140
}
139141
}
140142

141-
for (auto *sym : explicitlyPrivatizedSymbols)
142-
allPrivatizedSymbols.insert(sym);
143-
144143
if (hasCollapse && hasLastPrivateOp)
145144
TODO(converter.getCurrentLocation(), "Collapse clause with lastprivate");
146145
}
@@ -150,7 +149,7 @@ bool DataSharingProcessor::needBarrier() {
150149
// initialization of firstprivate variables and post-update of lastprivate
151150
// variables.
152151
// Emit implicit barrier for linear clause. Maybe on somewhere else.
153-
for (const semantics::Symbol *sym : allPrivatizedSymbols) {
152+
for (const semantics::Symbol *sym : privatizedSymbols) {
154153
if (sym->test(semantics::Symbol::Flag::OmpFirstPrivate) &&
155154
sym->test(semantics::Symbol::Flag::OmpLastPrivate))
156155
return true;
@@ -284,40 +283,10 @@ void DataSharingProcessor::collectSymbolsInNestedRegions(
284283
if (nestedEval.isConstruct())
285284
// Recursively look for OpenMP constructs within `nestedEval`'s region
286285
collectSymbolsInNestedRegions(nestedEval, flag, symbolsInNestedRegions);
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-
}
286+
else
287+
converter.collectSymbolSet(nestedEval, symbolsInNestedRegions, flag,
288+
/*collectSymbols=*/true,
289+
/*collectHostAssociatedSymbols=*/false);
321290
}
322291
}
323292
}
@@ -353,39 +322,24 @@ void DataSharingProcessor::collectSymbols(
353322
converter.collectSymbolSet(eval, allSymbols, flag,
354323
/*collectSymbols=*/true,
355324
/*collectHostAssociatedSymbols=*/true);
356-
357325
llvm::SetVector<const semantics::Symbol *> symbolsInNestedRegions;
358326
collectSymbolsInNestedRegions(eval, flag, symbolsInNestedRegions);
359327
// Filter-out symbols that must not be privatized.
360328
bool collectImplicit = flag == semantics::Symbol::Flag::OmpImplicit;
361-
bool collectPreDetermined = flag == semantics::Symbol::Flag::OmpPreDetermined;
362-
363329
auto isPrivatizable = [](const semantics::Symbol &sym) -> bool {
364330
return !semantics::IsProcedure(sym) &&
365331
!sym.GetUltimate().has<semantics::DerivedTypeDetails>() &&
366332
!sym.GetUltimate().has<semantics::NamelistDetails>() &&
367333
!semantics::IsImpliedDoIndex(sym.GetUltimate());
368334
};
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-
381335
for (const auto *sym : allSymbols) {
382336
assert(curScope && "couldn't find current scope");
383337
if (isPrivatizable(*sym) && !symbolsInNestedRegions.contains(sym) &&
384-
!explicitlyPrivatizedSymbols.contains(sym) &&
385-
shouldCollectSymbol(sym) && clauseScopes.contains(&sym->owner())) {
386-
allPrivatizedSymbols.insert(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()))
387342
symbols.insert(sym);
388-
}
389343
}
390344
}
391345

@@ -409,16 +363,10 @@ void DataSharingProcessor::collectImplicitSymbols() {
409363
collectSymbols(semantics::Symbol::Flag::OmpImplicit, implicitSymbols);
410364
}
411365

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

431379
void DataSharingProcessor::copyLastPrivatize(mlir::Operation *op) {
432380
insertLastPrivateCompare(op);
433-
for (const semantics::Symbol *sym : allPrivatizedSymbols)
381+
for (const semantics::Symbol *sym : privatizedSymbols)
434382
if (const auto *commonDet =
435383
sym->detailsIf<semantics::CommonBlockDetails>()) {
436384
for (const auto &mem : commonDet->objects()) {
@@ -441,6 +389,20 @@ void DataSharingProcessor::copyLastPrivatize(mlir::Operation *op) {
441389
}
442390
}
443391

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+
444406
void DataSharingProcessor::doPrivatize(
445407
const semantics::Symbol *sym, mlir::omp::PrivateClauseOps *clauseOps,
446408
llvm::SmallVectorImpl<const semantics::Symbol *> *privateSyms) {

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

Lines changed: 1 addition & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -37,20 +37,16 @@ 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 *> explicitlyPrivatizedSymbols;
40+
llvm::SetVector<const semantics::Symbol *> privatizedSymbols;
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-
4643
llvm::DenseMap<const semantics::Symbol *, mlir::omp::PrivateClauseOp>
4744
symToPrivatizer;
4845
lower::AbstractConverter &converter;
4946
semantics::SemanticsContext &semaCtx;
5047
fir::FirOpBuilder &firOpBuilder;
5148
omp::List<omp::Clause> clauses;
5249
lower::pft::Evaluation &eval;
53-
bool shouldCollectPreDeterminedSymbols;
5450
bool useDelayedPrivatization;
5551
lower::SymMap *symTable;
5652

@@ -67,7 +63,6 @@ class DataSharingProcessor {
6763
void insertBarrier();
6864
void collectDefaultSymbols();
6965
void collectImplicitSymbols();
70-
void collectPreDeterminedSymbols();
7166
void privatize(mlir::omp::PrivateClauseOps *clauseOps,
7267
llvm::SmallVectorImpl<const semantics::Symbol *> *privateSyms);
7368
void defaultPrivatize(
@@ -95,12 +90,10 @@ class DataSharingProcessor {
9590
semantics::SemanticsContext &semaCtx,
9691
const List<Clause> &clauses,
9792
lower::pft::Evaluation &eval,
98-
bool shouldCollectPreDeterminedSymbols,
9993
bool useDelayedPrivatization = false,
10094
lower::SymMap *symTable = nullptr)
10195
: hasLastPrivateOp(false), converter(converter), semaCtx(semaCtx),
10296
firOpBuilder(converter.getFirOpBuilder()), clauses(clauses), eval(eval),
103-
shouldCollectPreDeterminedSymbols(shouldCollectPreDeterminedSymbols),
10497
useDelayedPrivatization(useDelayedPrivatization), symTable(symTable) {}
10598

10699
// Privatisation is split into two steps.

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

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -123,9 +123,4 @@ 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-
}
131126
} // namespace Fortran::lower::omp

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

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -46,9 +46,6 @@ 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);
5249
} // namespace Fortran::lower::omp
5350

5451
#endif // FORTRAN_LOWER_OPENMP_DECOMPOSER_H

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

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

217217
mlir::Type tempTy = converter.genType(*sym);
218-
219-
assert(converter.isPresentShallowLookup(*sym) &&
220-
"Expected symbol to be in symbol table.");
221-
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);
222224
firOpBuilder.restoreInsertionPoint(insPt);
223225
mlir::Value cvtVal = firOpBuilder.createConvert(loc, tempTy, indexVal);
224226
mlir::Operation *storeOp = firOpBuilder.create<fir::StoreOp>(
@@ -578,8 +580,7 @@ static void createBodyOfOp(mlir::Operation &op, const OpWithBodyGenInfo &info,
578580
std::optional<DataSharingProcessor> tempDsp;
579581
if (privatize) {
580582
if (!info.dsp) {
581-
tempDsp.emplace(info.converter, info.semaCtx, *info.clauses, info.eval,
582-
Fortran::lower::omp::isLastItemInQueue(item, queue));
583+
tempDsp.emplace(info.converter, info.semaCtx, *info.clauses, info.eval);
583584
tempDsp->processStep1();
584585
}
585586
}
@@ -1315,7 +1316,6 @@ genParallelOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
13151316

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

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

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

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

14671464
lower::StatementContext stmtCtx;
@@ -1499,7 +1496,6 @@ genSimdOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
14991496
.setGenRegionEntryCb(ivCallback),
15001497
queue, item);
15011498

1502-
symTable.popScope();
15031499
return simdOp;
15041500
}
15051501

@@ -1765,9 +1761,7 @@ genWsloopOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
17651761
mlir::Location loc, const ConstructQueue &queue,
17661762
ConstructQueue::iterator item) {
17671763
fir::FirOpBuilder &firOpBuilder = converter.getFirOpBuilder();
1768-
symTable.pushScope();
1769-
DataSharingProcessor dsp(converter, semaCtx, item->clauses, eval,
1770-
lower::omp::isLastItemInQueue(item, queue));
1764+
DataSharingProcessor dsp(converter, semaCtx, item->clauses, eval);
17711765
dsp.processStep1();
17721766

17731767
lower::StatementContext stmtCtx;
@@ -1810,7 +1804,6 @@ genWsloopOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
18101804
.setReductions(&reductionSyms, &reductionTypes)
18111805
.setGenRegionEntryCb(ivCallback),
18121806
queue, item);
1813-
symTable.popScope();
18141807
return wsloopOp;
18151808
}
18161809

‎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 {bindc_name = "x", pinned, {{.*}}}
11+
! CHECK: %[[TEMP:.*]] = fir.alloca i32 {adapt.valuebyref, 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)