Skip to content

Commit f0ccde9

Browse files
tblahergawy
authored andcommitted
[mlir][OpenMP][flang] make private variable allocation implicit in omp.private (llvm#124019)
The intention of this work is to give MLIR->LLVMIR conversion freedom to control how the private variable is allocated so that it can be allocated on the stack in ordinary cases or as part of a structure used to give closure context for tasks which might outlive the current stack frame. See RFC: https://discourse.llvm.org/t/rfc-openmp-supporting-delayed-task-execution-with-firstprivate-variables/83084 For example, a privatizer for an integer used to look like ```mlir omp.private {type = private} @x.privatizer : !fir.ref<i32> alloc { ^bb0(%arg0: !fir.ref<i32>): %0 = ... allocate proper memory for the private clone ... omp.yield(%0 : !fir.ref<i32>) } ``` After this change, allocation become implicit in the operation: ```mlir omp.private {type = private} @x.privatizer : i32 ``` For more complex types that require initialization after allocation, an init region can be used: ``` mlir omp.private {type = private} @x.privatizer : !some.type init { ^bb0(%arg0: !some.pointer<!some.type>, %arg1: !some.pointer<!some.type>): // initialize %arg1, using %arg0 as a mold for allocations omp.yield(%arg1 : !some.pointer<!some.type>) } dealloc { ^bb0(%arg0: !some.pointer<!some.type>): ... deallocate memory allocated by the init region ... omp.yield } ``` This patch lays the groundwork for delayed task execution but is not enough on its own. After this patch all gfortran tests which previously passed still pass. There are the following changes to the Fujitsu test suite: - 0380_0009 and 0435_0009 are fixed - 0688_0041 now fails at runtime. This patch is testing firstprivate variables with tasks. Previously we got lucky with the undefined behavior and won the race. After these changes we no longer get lucky. This patch lays the groundwork for a proper fix for this issue. In flang the lowering re-uses the existing lowering used for reduction init and dealloc regions. In flang, before this patch we hit a TODO with the same wording when generating the copy region for firstprivate polymorphic variables. After this patch the box-like fir.class is passed by reference into the copy region, leading to a different path that didn't hit that old TODO but the generated code still didn't work so I added a new TODO in DataSharingProcessor.
1 parent dc7d034 commit f0ccde9

File tree

77 files changed

+1401
-1167
lines changed

Some content is hidden

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

77 files changed

+1401
-1167
lines changed

flang/lib/Lower/OpenMP/DataSharingProcessor.cpp

Lines changed: 125 additions & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -17,9 +17,12 @@
1717
#include "flang/Lower/OpenMP/Utils.h"
1818
#include "flang/Lower/PFTBuilder.h"
1919
#include "flang/Lower/SymbolMap.h"
20+
#include "flang/Optimizer/Builder/BoxValue.h"
2021
#include "flang/Optimizer/Builder/HLFIRTools.h"
2122
#include "flang/Optimizer/Builder/Todo.h"
23+
#include "flang/Optimizer/HLFIR/HLFIRDialect.h"
2224
#include "flang/Optimizer/HLFIR/HLFIROps.h"
25+
#include "flang/Semantics/attr.h"
2326
#include "flang/Semantics/tools.h"
2427

2528
namespace Fortran {
@@ -91,35 +94,65 @@ void DataSharingProcessor::insertDeallocs() {
9194
converter.createHostAssociateVarCloneDealloc(*sym);
9295
continue;
9396
}
94-
95-
lower::SymbolBox hsb = converter.lookupOneLevelUpSymbol(*sym);
96-
assert(hsb && "Host symbol box not found");
97-
mlir::Type symType = hsb.getAddr().getType();
98-
mlir::Location symLoc = hsb.getAddr().getLoc();
99-
fir::ExtendedValue symExV = converter.getSymbolExtendedValue(*sym);
100-
mlir::omp::PrivateClauseOp privatizer = symToPrivatizer.at(sym);
101-
102-
lower::SymMapScope scope(symTable);
103-
mlir::OpBuilder::InsertionGuard guard(firOpBuilder);
104-
105-
mlir::Region &deallocRegion = privatizer.getDeallocRegion();
106-
fir::FirOpBuilder &firOpBuilder = converter.getFirOpBuilder();
107-
mlir::Block *deallocEntryBlock = firOpBuilder.createBlock(
108-
&deallocRegion, /*insertPt=*/{}, symType, symLoc);
109-
110-
firOpBuilder.setInsertionPointToEnd(deallocEntryBlock);
111-
symTable.addSymbol(*sym,
112-
fir::substBase(symExV, deallocRegion.getArgument(0)));
113-
114-
converter.createHostAssociateVarCloneDealloc(*sym);
115-
firOpBuilder.create<mlir::omp::YieldOp>(hsb.getAddr().getLoc());
97+
// For delayed privatization deallocs are created by
98+
// populateByRefInitAndCleanupRegions
11699
}
117100
}
118101

119102
void DataSharingProcessor::cloneSymbol(const semantics::Symbol *sym) {
120103
bool isFirstPrivate = sym->test(semantics::Symbol::Flag::OmpFirstPrivate);
121-
bool success = converter.createHostAssociateVarClone(
122-
*sym, /*skipDefaultInit=*/isFirstPrivate);
104+
105+
// If we are doing eager-privatization on a symbol created using delayed
106+
// privatization there could be incompatible types here e.g.
107+
// fir.ref<fir.box<fir.array<>>>
108+
bool success = [&]() -> bool {
109+
const auto *details =
110+
sym->detailsIf<Fortran::semantics::HostAssocDetails>();
111+
assert(details && "No host-association found");
112+
const Fortran::semantics::Symbol &hsym = details->symbol();
113+
mlir::Value addr = converter.getSymbolAddress(hsym);
114+
115+
if (auto refTy = mlir::dyn_cast<fir::ReferenceType>(addr.getType())) {
116+
if (auto boxTy = mlir::dyn_cast<fir::BoxType>(refTy.getElementType())) {
117+
if (auto arrayTy =
118+
mlir::dyn_cast<fir::SequenceType>(boxTy.getElementType())) {
119+
// FirConverter/fir::ExtendedValue considers all references to boxes
120+
// as mutable boxes. Outside of OpenMP it doesn't make sense to have a
121+
// mutable box of an array. Work around this here by loading the
122+
// reference so it is a normal boxed array.
123+
fir::FirOpBuilder &builder = converter.getFirOpBuilder();
124+
mlir::Location loc = converter.genLocation(hsym.name());
125+
fir::ExtendedValue hexv = converter.getSymbolExtendedValue(hsym);
126+
127+
llvm::SmallVector<mlir::Value> extents =
128+
fir::factory::getExtents(loc, builder, hexv);
129+
130+
// TODO: uniqName, name
131+
mlir::Value allocVal =
132+
builder.allocateLocal(loc, arrayTy, /*uniqName=*/"",
133+
/*name=*/"", extents, /*typeParams=*/{},
134+
sym->GetUltimate().attrs().test(
135+
Fortran::semantics::Attr::TARGET));
136+
mlir::Value shape = builder.genShape(loc, extents);
137+
mlir::Value box = builder.createBox(loc, boxTy, allocVal, shape,
138+
nullptr, {}, nullptr);
139+
140+
// This can't be a CharArrayBoxValue because otherwise
141+
// boxTy.getElementType() would be a character type.
142+
// Assume the array element type isn't polymorphic because we are
143+
// privatizing.
144+
fir::ExtendedValue newExv = fir::ArrayBoxValue{box, extents};
145+
146+
converter.bindSymbol(*sym, newExv);
147+
return true;
148+
}
149+
}
150+
}
151+
152+
// Normal case:
153+
return converter.createHostAssociateVarClone(
154+
*sym, /*skipDefaultInit=*/isFirstPrivate);
155+
}();
123156
(void)success;
124157
assert(success && "Privatization failed due to existing binding");
125158

@@ -138,7 +171,7 @@ void DataSharingProcessor::cloneSymbol(const semantics::Symbol *sym) {
138171

139172
if (needInitClone()) {
140173
Fortran::lower::initializeCloneAtRuntime(converter, *sym, symTable);
141-
callsInitClone = true;
174+
mightHaveReadHostSym = true;
142175
}
143176
}
144177

@@ -190,7 +223,8 @@ bool DataSharingProcessor::needBarrier() {
190223
// Emit implicit barrier for linear clause. Maybe on somewhere else.
191224
for (const semantics::Symbol *sym : allPrivatizedSymbols) {
192225
if (sym->test(semantics::Symbol::Flag::OmpLastPrivate) &&
193-
(sym->test(semantics::Symbol::Flag::OmpFirstPrivate) || callsInitClone))
226+
(sym->test(semantics::Symbol::Flag::OmpFirstPrivate) ||
227+
mightHaveReadHostSym))
194228
return true;
195229
}
196230
return false;
@@ -475,15 +509,47 @@ void DataSharingProcessor::doPrivatize(const semantics::Symbol *sym,
475509
lower::SymbolBox hsb = converter.lookupOneLevelUpSymbol(*sym);
476510
assert(hsb && "Host symbol box not found");
477511

478-
mlir::Type symType = hsb.getAddr().getType();
479512
mlir::Location symLoc = hsb.getAddr().getLoc();
480513
std::string privatizerName = sym->name().ToString() + ".privatizer";
481514
bool isFirstPrivate = sym->test(semantics::Symbol::Flag::OmpFirstPrivate);
482515

516+
mlir::Value privVal = hsb.getAddr();
517+
mlir::Type allocType = privVal.getType();
518+
if (!mlir::isa<fir::PointerType>(privVal.getType()))
519+
allocType = fir::unwrapRefType(privVal.getType());
520+
521+
if (auto poly = mlir::dyn_cast<fir::ClassType>(allocType)) {
522+
if (!mlir::isa<fir::PointerType>(poly.getEleTy()) && isFirstPrivate)
523+
TODO(symLoc, "create polymorphic host associated copy");
524+
}
525+
526+
// fir.array<> cannot be converted to any single llvm type and fir helpers
527+
// are not available in openmp to llvmir translation so we cannot generate
528+
// an alloca for a fir.array type there. Get around this by boxing all
529+
// arrays.
530+
if (mlir::isa<fir::SequenceType>(allocType)) {
531+
hlfir::Entity entity{hsb.getAddr()};
532+
entity = genVariableBox(symLoc, firOpBuilder, entity);
533+
privVal = entity.getBase();
534+
allocType = privVal.getType();
535+
}
536+
537+
if (mlir::isa<fir::BaseBoxType>(privVal.getType())) {
538+
// Boxes should be passed by reference into nested regions:
539+
auto oldIP = firOpBuilder.saveInsertionPoint();
540+
firOpBuilder.setInsertionPointToStart(firOpBuilder.getAllocaBlock());
541+
auto alloca = firOpBuilder.create<fir::AllocaOp>(symLoc, privVal.getType());
542+
firOpBuilder.restoreInsertionPoint(oldIP);
543+
firOpBuilder.create<fir::StoreOp>(symLoc, privVal, alloca);
544+
privVal = alloca;
545+
}
546+
547+
mlir::Type argType = privVal.getType();
548+
483549
mlir::omp::PrivateClauseOp privatizerOp = [&]() {
484550
auto moduleOp = firOpBuilder.getModule();
485551
auto uniquePrivatizerName = fir::getTypeAsString(
486-
symType, converter.getKindMap(),
552+
allocType, converter.getKindMap(),
487553
converter.mangleName(*sym) +
488554
(isFirstPrivate ? "_firstprivate" : "_private"));
489555

@@ -495,44 +561,40 @@ void DataSharingProcessor::doPrivatize(const semantics::Symbol *sym,
495561
mlir::OpBuilder::InsertionGuard guard(firOpBuilder);
496562
firOpBuilder.setInsertionPointToStart(moduleOp.getBody());
497563
auto result = firOpBuilder.create<mlir::omp::PrivateClauseOp>(
498-
symLoc, uniquePrivatizerName, symType,
564+
symLoc, uniquePrivatizerName, allocType,
499565
isFirstPrivate ? mlir::omp::DataSharingClauseType::FirstPrivate
500566
: mlir::omp::DataSharingClauseType::Private);
501567
fir::ExtendedValue symExV = converter.getSymbolExtendedValue(*sym);
502568
lower::SymMapScope outerScope(symTable);
503569

504-
// Populate the `alloc` region.
505-
{
506-
mlir::Region &allocRegion = result.getAllocRegion();
507-
mlir::Block *allocEntryBlock = firOpBuilder.createBlock(
508-
&allocRegion, /*insertPt=*/{}, symType, symLoc);
509-
510-
firOpBuilder.setInsertionPointToEnd(allocEntryBlock);
511-
512-
fir::ExtendedValue localExV =
513-
hlfir::translateToExtendedValue(
514-
symLoc, firOpBuilder, hlfir::Entity{allocRegion.getArgument(0)},
515-
/*contiguousHint=*/
516-
evaluate::IsSimplyContiguous(*sym, converter.getFoldingContext()))
517-
.first;
518-
519-
symTable.addSymbol(*sym, localExV);
520-
lower::SymMapScope innerScope(symTable);
521-
cloneSymbol(sym);
522-
mlir::Value cloneAddr = symTable.shallowLookupSymbol(*sym).getAddr();
523-
mlir::Type cloneType = cloneAddr.getType();
524-
525-
// A `convert` op is required for variables that are storage associated
526-
// via `equivalence`. The problem is that these variables are declared as
527-
// `fir.ptr`s while their privatized storage is declared as `fir.ref`,
528-
// therefore we convert to proper symbol type.
529-
mlir::Value yieldedValue =
530-
(symType == cloneType) ? cloneAddr
531-
: firOpBuilder.createConvert(
532-
cloneAddr.getLoc(), symType, cloneAddr);
533-
534-
firOpBuilder.create<mlir::omp::YieldOp>(hsb.getAddr().getLoc(),
535-
yieldedValue);
570+
// Populate the `init` region.
571+
// We need to initialize in the following cases:
572+
// 1. The allocation was for a derived type which requires initialization
573+
// (this can be skipped if it will be initialized anyway by the copy
574+
// region, unless the derived type has allocatable components)
575+
// 2. The allocation was for any kind of box
576+
// 3. The allocation was for a boxed character
577+
const bool needsInitialization =
578+
(Fortran::lower::hasDefaultInitialization(sym->GetUltimate()) &&
579+
(!isFirstPrivate || hlfir::mayHaveAllocatableComponent(allocType))) ||
580+
mlir::isa<fir::BaseBoxType>(allocType) ||
581+
mlir::isa<fir::BoxCharType>(allocType);
582+
if (needsInitialization) {
583+
mlir::Region &initRegion = result.getInitRegion();
584+
mlir::Block *initBlock = firOpBuilder.createBlock(
585+
&initRegion, /*insertPt=*/{}, {argType, argType}, {symLoc, symLoc});
586+
587+
populateByRefInitAndCleanupRegions(
588+
converter, symLoc, argType, /*scalarInitValue=*/nullptr, initBlock,
589+
result.getInitPrivateArg(), result.getInitMoldArg(),
590+
result.getDeallocRegion(),
591+
isFirstPrivate ? DeclOperationKind::FirstPrivate
592+
: DeclOperationKind::Private,
593+
sym);
594+
// TODO: currently there are false positives from dead uses of the mold
595+
// arg
596+
if (!result.getInitMoldArg().getUses().empty())
597+
mightHaveReadHostSym = true;
536598
}
537599

538600
// Populate the `copy` region if this is a `firstprivate`.
@@ -541,7 +603,7 @@ void DataSharingProcessor::doPrivatize(const semantics::Symbol *sym,
541603
// First block argument corresponding to the original/host value while
542604
// second block argument corresponding to the privatized value.
543605
mlir::Block *copyEntryBlock = firOpBuilder.createBlock(
544-
&copyRegion, /*insertPt=*/{}, {symType, symType}, {symLoc, symLoc});
606+
&copyRegion, /*insertPt=*/{}, {argType, argType}, {symLoc, symLoc});
545607
firOpBuilder.setInsertionPointToEnd(copyEntryBlock);
546608

547609
auto addSymbol = [&](unsigned argIdx, bool force = false) {
@@ -572,7 +634,7 @@ void DataSharingProcessor::doPrivatize(const semantics::Symbol *sym,
572634

573635
if (clauseOps) {
574636
clauseOps->privateSyms.push_back(mlir::SymbolRefAttr::get(privatizerOp));
575-
clauseOps->privateVars.push_back(hsb.getAddr());
637+
clauseOps->privateVars.push_back(privVal);
576638
}
577639

578640
symToPrivatizer[sym] = privatizerOp;

flang/lib/Lower/OpenMP/DataSharingProcessor.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,7 @@ class DataSharingProcessor {
8686
lower::pft::Evaluation &eval;
8787
bool shouldCollectPreDeterminedSymbols;
8888
bool useDelayedPrivatization;
89-
bool callsInitClone = false;
89+
bool mightHaveReadHostSym = false;
9090
lower::SymMap &symTable;
9191
OMPConstructSymbolVisitor visitor;
9292
bool privatizationDone = false;

0 commit comments

Comments
 (0)