Skip to content

[Flang][OpenMP] Make implicitly captured scalars fully firstprivatized #147442

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 5, 2025
Merged

[Flang][OpenMP] Make implicitly captured scalars fully firstprivatized #147442

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 28 additions & 5 deletions flang/lib/Lower/OpenMP/DataSharingProcessor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,16 +43,26 @@ bool DataSharingProcessor::OMPConstructSymbolVisitor::isSymbolDefineBy(
[](const auto &functionParserNode) { return false; }});
}

static bool isConstructWithTopLevelTarget(lower::pft::Evaluation &eval) {
const auto *ompEval = eval.getIf<parser::OpenMPConstruct>();
if (ompEval) {
auto dir = parser::omp::GetOmpDirectiveName(ompEval).v;
if (llvm::omp::topTargetSet.test(dir))
return true;
}
return false;
}

DataSharingProcessor::DataSharingProcessor(
lower::AbstractConverter &converter, semantics::SemanticsContext &semaCtx,
const List<Clause> &clauses, lower::pft::Evaluation &eval,
bool shouldCollectPreDeterminedSymbols, bool useDelayedPrivatization,
lower::SymMap &symTable)
lower::SymMap &symTable, bool isTargetPrivatization)
: converter(converter), semaCtx(semaCtx),
firOpBuilder(converter.getFirOpBuilder()), clauses(clauses), eval(eval),
shouldCollectPreDeterminedSymbols(shouldCollectPreDeterminedSymbols),
useDelayedPrivatization(useDelayedPrivatization), symTable(symTable),
visitor(semaCtx) {
isTargetPrivatization(isTargetPrivatization), visitor(semaCtx) {
eval.visit([&](const auto &functionParserNode) {
parser::Walk(functionParserNode, visitor);
});
Expand All @@ -62,10 +72,12 @@ DataSharingProcessor::DataSharingProcessor(lower::AbstractConverter &converter,
semantics::SemanticsContext &semaCtx,
lower::pft::Evaluation &eval,
bool useDelayedPrivatization,
lower::SymMap &symTable)
lower::SymMap &symTable,
bool isTargetPrivatization)
: DataSharingProcessor(converter, semaCtx, {}, eval,
/*shouldCollectPreDeterminedSymols=*/false,
useDelayedPrivatization, symTable) {}
useDelayedPrivatization, symTable,
isTargetPrivatization) {}

void DataSharingProcessor::processStep1(
mlir::omp::PrivateClauseOps *clauseOps) {
Expand Down Expand Up @@ -552,8 +564,19 @@ void DataSharingProcessor::collectSymbols(
};

auto shouldCollectSymbol = [&](const semantics::Symbol *sym) {
if (collectImplicit)
if (collectImplicit) {
// If we're a combined construct with a target region, implicit
// firstprivate captures, should only belong to the target region
// and not be added/captured by later directives. Parallel regions
// will likely want the same captures to be shared and for SIMD it's
// illegal to have firstprivate clauses.
if (isConstructWithTopLevelTarget(eval) && !isTargetPrivatization &&
sym->test(semantics::Symbol::Flag::OmpFirstPrivate)) {
return false;
}

return sym->test(semantics::Symbol::Flag::OmpImplicit);
}

if (collectPreDetermined)
return sym->test(semantics::Symbol::Flag::OmpPreDetermined);
Expand Down
7 changes: 5 additions & 2 deletions flang/lib/Lower/OpenMP/DataSharingProcessor.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ class DataSharingProcessor {
bool useDelayedPrivatization;
llvm::SmallSet<const semantics::Symbol *, 16> mightHaveReadHostSym;
lower::SymMap &symTable;
bool isTargetPrivatization;
OMPConstructSymbolVisitor visitor;

bool needBarrier();
Expand Down Expand Up @@ -130,12 +131,14 @@ class DataSharingProcessor {
const List<Clause> &clauses,
lower::pft::Evaluation &eval,
bool shouldCollectPreDeterminedSymbols,
bool useDelayedPrivatization, lower::SymMap &symTable);
bool useDelayedPrivatization, lower::SymMap &symTable,
bool isTargetPrivatization = false);

DataSharingProcessor(lower::AbstractConverter &converter,
semantics::SemanticsContext &semaCtx,
lower::pft::Evaluation &eval,
bool useDelayedPrivatization, lower::SymMap &symTable);
bool useDelayedPrivatization, lower::SymMap &symTable,
bool isTargetPrivatization = false);

// Privatisation is split into two steps.
// Step1 performs cloning of all privatisation clauses and copying for
Expand Down
48 changes: 35 additions & 13 deletions flang/lib/Lower/OpenMP/OpenMP.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2473,6 +2473,33 @@ genSingleOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
queue, item, clauseOps);
}

static bool isDuplicateMappedSymbol(
const semantics::Symbol &sym,
const llvm::SetVector<const semantics::Symbol *> &privatizedSyms,
const llvm::SmallVectorImpl<const semantics::Symbol *> &hasDevSyms,
const llvm::SmallVectorImpl<const semantics::Symbol *> &mappedSyms) {
llvm::SmallVector<const semantics::Symbol *> concatSyms;
concatSyms.reserve(privatizedSyms.size() + hasDevSyms.size() +
mappedSyms.size());
concatSyms.append(privatizedSyms.begin(), privatizedSyms.end());
concatSyms.append(hasDevSyms.begin(), hasDevSyms.end());
concatSyms.append(mappedSyms.begin(), mappedSyms.end());

auto checkSymbol = [&](const semantics::Symbol &checkSym) {
return std::any_of(concatSyms.begin(), concatSyms.end(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, a nit really. But, doens't this second check subsume the check above?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be equivalent due to the checkSymbol call at L2500, but without it I don't think it is, since we have L2500 though I'll see if removing it works fine! Thank you for spotting that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, that's exactly what I meant. I meant to mark this comment on L2500 instead of L2489.

[&](auto v) { return v->GetUltimate() == checkSym; });
};

if (checkSymbol(sym))
return true;

const auto *hostAssoc{sym.detailsIf<semantics::HostAssocDetails>()};
if (hostAssoc && checkSymbol(hostAssoc->symbol()))
return true;

return checkSymbol(sym.GetUltimate());
}

static mlir::omp::TargetOp
genTargetOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
lower::StatementContext &stmtCtx,
Expand All @@ -2499,7 +2526,8 @@ genTargetOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
DataSharingProcessor dsp(converter, semaCtx, item->clauses, eval,
/*shouldCollectPreDeterminedSymbols=*/
lower::omp::isLastItemInQueue(item, queue),
/*useDelayedPrivatization=*/true, symTable);
/*useDelayedPrivatization=*/true, symTable,
/*isTargetPrivitization=*/true);
dsp.processStep1(&clauseOps);

// 5.8.1 Implicit Data-Mapping Attribute Rules
Expand All @@ -2508,17 +2536,6 @@ genTargetOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
// attribute clauses (neither data-sharing; e.g. `private`, nor `map`
// clauses).
auto captureImplicitMap = [&](const semantics::Symbol &sym) {
if (dsp.getAllSymbolsToPrivatize().contains(&sym))
return;

// Skip parameters/constants as they do not need to be mapped.
if (semantics::IsNamedConstant(sym))
return;

// These symbols are mapped individually in processHasDeviceAddr.
if (llvm::is_contained(hasDeviceAddrSyms, &sym))
return;

// Structure component symbols don't have bindings, and can only be
// explicitly mapped individually. If a member is captured implicitly
// we map the entirety of the derived type when we find its symbol.
Expand All @@ -2540,7 +2557,12 @@ genTargetOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
if (!converter.getSymbolAddress(sym))
return;

if (!llvm::is_contained(mapSyms, &sym)) {
// Skip parameters/constants as they do not need to be mapped.
if (semantics::IsNamedConstant(sym))
return;

if (!isDuplicateMappedSymbol(sym, dsp.getAllSymbolsToPrivatize(),
hasDeviceAddrSyms, mapSyms)) {
if (const auto *details =
sym.template detailsIf<semantics::HostAssocDetails>())
converter.copySymbolBinding(details->symbol(), sym);
Expand Down
26 changes: 24 additions & 2 deletions flang/lib/Optimizer/OpenMP/MapsForPrivatizedSymbols.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,20 @@ class MapsForPrivatizedSymbolsPass

omp::MapInfoOp createMapInfo(Location loc, Value var,
fir::FirOpBuilder &builder) {
// Check if a value of type `type` can be passed to the kernel by value.
// All kernel parameters are of pointer type, so if the value can be
// represented inside of a pointer, then it can be passed by value.
auto canPassByValue = [&](mlir::Type type) {
const mlir::DataLayout &dl = builder.getDataLayout();
mlir::Type ptrTy = mlir::LLVM::LLVMPointerType::get(builder.getContext());
uint64_t ptrSize = dl.getTypeSize(ptrTy);
uint64_t ptrAlign = dl.getTypePreferredAlignment(ptrTy);

auto [size, align] = fir::getTypeSizeAndAlignmentOrCrash(
loc, type, dl, builder.getKindMap());
return size <= ptrSize && align <= ptrAlign;
};

uint64_t mapTypeTo = static_cast<
std::underlying_type_t<llvm::omp::OpenMPOffloadMappingFlags>>(
llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_TO);
Expand Down Expand Up @@ -94,14 +108,22 @@ class MapsForPrivatizedSymbolsPass
if (needsBoundsOps(varPtr))
genBoundsOps(builder, varPtr, boundsOps);

mlir::omp::VariableCaptureKind captureKind =
mlir::omp::VariableCaptureKind::ByRef;
if (fir::isa_trivial(fir::unwrapRefType(varPtr.getType())) ||
fir::isa_char(fir::unwrapRefType(varPtr.getType()))) {
if (canPassByValue(fir::unwrapRefType(varPtr.getType()))) {
captureKind = mlir::omp::VariableCaptureKind::ByCopy;
}
}

return omp::MapInfoOp::create(
builder, loc, varPtr.getType(), varPtr,
TypeAttr::get(llvm::cast<omp::PointerLikeType>(varPtr.getType())
.getElementType()),
builder.getIntegerAttr(builder.getIntegerType(64, /*isSigned=*/false),
mapTypeTo),
builder.getAttr<omp::VariableCaptureKindAttr>(
omp::VariableCaptureKind::ByRef),
builder.getAttr<omp::VariableCaptureKindAttr>(captureKind),
/*varPtrPtr=*/Value{},
/*members=*/SmallVector<Value>{},
/*member_index=*/mlir::ArrayAttr{},
Expand Down
116 changes: 114 additions & 2 deletions flang/lib/Semantics/resolve-directives.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
#include "flang/Semantics/openmp-modifiers.h"
#include "flang/Semantics/symbol.h"
#include "flang/Semantics/tools.h"
#include "flang/Support/Flags.h"
#include "llvm/Frontend/OpenMP/OMP.h.inc"
#include "llvm/Support/Debug.h"
#include <list>
Expand Down Expand Up @@ -56,6 +57,10 @@ template <typename T> class DirectiveAttributeVisitor {
Scope &scope;
Symbol::Flag defaultDSA{Symbol::Flag::AccShared}; // TODOACC
std::map<const Symbol *, Symbol::Flag> objectWithDSA;
std::map<parser::OmpVariableCategory::Value,
parser::OmpDefaultmapClause::ImplicitBehavior>
defaultMap;

bool withinConstruct{false};
std::int64_t associatedLoopLevel{0};
};
Expand All @@ -80,6 +85,10 @@ template <typename T> class DirectiveAttributeVisitor {
GetContext().directiveSource = dir;
}
Scope &currScope() { return GetContext().scope; }
void AddContextDefaultmapBehaviour(parser::OmpVariableCategory::Value VarCat,
parser::OmpDefaultmapClause::ImplicitBehavior ImpBehav) {
GetContext().defaultMap[VarCat] = ImpBehav;
}
void SetContextDefaultDSA(Symbol::Flag flag) {
GetContext().defaultDSA = flag;
}
Expand Down Expand Up @@ -557,6 +566,7 @@ class OmpAttributeVisitor : DirectiveAttributeVisitor<llvm::omp::Directive> {
ResolveOmpObjectList(x.v, Symbol::Flag::OmpExclusiveScan);
return false;
}
void Post(const parser::OmpClause::Defaultmap &);
void Post(const parser::OmpDefaultClause &);
bool Pre(const parser::OmpClause::Shared &x) {
ResolveOmpObjectList(x.v, Symbol::Flag::OmpShared);
Expand Down Expand Up @@ -810,6 +820,11 @@ class OmpAttributeVisitor : DirectiveAttributeVisitor<llvm::omp::Directive> {
Symbol::Flag::OmpLastPrivate, Symbol::Flag::OmpReduction,
Symbol::Flag::OmpLinear};

Symbol::Flags dataMappingAttributeFlags{Symbol::Flag::OmpMapTo,
Symbol::Flag::OmpMapFrom, Symbol::Flag::OmpMapToFrom,
Symbol::Flag::OmpMapStorage, Symbol::Flag::OmpMapDelete,
Symbol::Flag::OmpIsDevicePtr, Symbol::Flag::OmpHasDeviceAddr};

Symbol::Flags privateDataSharingAttributeFlags{Symbol::Flag::OmpPrivate,
Symbol::Flag::OmpFirstPrivate, Symbol::Flag::OmpLastPrivate};

Expand Down Expand Up @@ -2214,6 +2229,28 @@ bool OmpAttributeVisitor::Pre(const parser::OpenMPAllocatorsConstruct &x) {
return true;
}

void OmpAttributeVisitor::Post(const parser::OmpClause::Defaultmap &x) {
using ImplicitBehavior = parser::OmpDefaultmapClause::ImplicitBehavior;
using VariableCategory = parser::OmpVariableCategory;

VariableCategory::Value varCategory;
ImplicitBehavior impBehavior;

if (!dirContext_.empty()) {
impBehavior = std::get<ImplicitBehavior>(x.v.t);

auto &modifiers{OmpGetModifiers(x.v)};
auto *maybeCategory{
OmpGetUniqueModifier<parser::OmpVariableCategory>(modifiers)};
if (maybeCategory)
varCategory = maybeCategory->v;
else
varCategory = VariableCategory::Value::All;

AddContextDefaultmapBehaviour(varCategory, impBehavior);
}
}

void OmpAttributeVisitor::Post(const parser::OmpDefaultClause &x) {
// The DEFAULT clause may also be used on METADIRECTIVE. In that case
// there is nothing to do.
Expand Down Expand Up @@ -2365,6 +2402,70 @@ static bool IsSymbolStaticStorageDuration(const Symbol &symbol) {
(ultSym.flags().test(Symbol::Flag::InCommonBlock));
}

static bool IsTargetCaptureImplicitlyFirstprivatizeable(const Symbol &symbol,
const Symbol::Flags &dsa, const Symbol::Flags &dataSharingAttributeFlags,
const Symbol::Flags &dataMappingAttributeFlags,
std::map<parser::OmpVariableCategory::Value,
parser::OmpDefaultmapClause::ImplicitBehavior>
defaultMap) {
// If a Defaultmap clause is present for the current target scope, and it has
// specified behaviour other than Firstprivate for scalars then we exit early,
// as it overrides the implicit Firstprivatization of scalars OpenMP rule.
if (!defaultMap.empty()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

< minor >: Could you please add a comment here to suggest that we exit early if we are dealing with a scalar and the defaultmap clause has set the implicit mapping behavior to something other than firstprivate.

Helps readability because the compound conditional is quite long.

if (llvm::is_contained(
defaultMap, parser::OmpVariableCategory::Value::All) &&
defaultMap[parser::OmpVariableCategory::Value::All] !=
parser::OmpDefaultmapClause::ImplicitBehavior::Firstprivate) {
return false;
}

if (llvm::is_contained(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check seems to subsume the check above. If that's correct, then we should lead with this check rather than the one before.

defaultMap, parser::OmpVariableCategory::Value::Scalar) &&
defaultMap[parser::OmpVariableCategory::Value::Scalar] !=
parser::OmpDefaultmapClause::ImplicitBehavior::Firstprivate) {
return false;
}
}

auto checkSymbol = [&](const Symbol &checkSym) {
// if we're associated with any other flags we skip implicit privitization
// for now. If we're an allocatable, pointer or declare target, we're not
// implicitly firstprivitizeable under OpenMP restrictions.
// TODO: Relax restriction as we progress privitization and further
// investigate the flags we can intermix with.
if (!(dsa & (dataSharingAttributeFlags | dataMappingAttributeFlags))
.none() ||
!checkSym.flags().none() || semantics::IsAssumedShape(checkSym) ||
semantics::IsAllocatableOrPointer(checkSym)) {
return false;
}

// It is default firstprivatizeable as far as the OpenMP specification is
// concerned if it is a non-array scalar type that has been implicitly
// captured in a target region
const auto *type{checkSym.GetType()};
if ((!checkSym.GetShape() || checkSym.GetShape()->empty()) &&
(type->category() ==
Fortran::semantics::DeclTypeSpec::Category::Numeric ||
type->category() ==
Fortran::semantics::DeclTypeSpec::Category::Logical ||
type->category() ==
Fortran::semantics::DeclTypeSpec::Category::Character)) {
return true;
}
return false;
};

if (checkSymbol(symbol)) {
const auto *hostAssoc{symbol.detailsIf<HostAssocDetails>()};
if (hostAssoc) {
return checkSymbol(hostAssoc->symbol());
}
return true;
}
return false;
}

void OmpAttributeVisitor::CreateImplicitSymbols(const Symbol *symbol) {
if (!IsPrivatizable(symbol)) {
return;
Expand Down Expand Up @@ -2456,7 +2557,7 @@ void OmpAttributeVisitor::CreateImplicitSymbols(const Symbol *symbol) {

bool taskGenDir = llvm::omp::taskGeneratingSet.test(dirContext.directive);
bool targetDir = llvm::omp::allTargetSet.test(dirContext.directive);
bool parallelDir = llvm::omp::allParallelSet.test(dirContext.directive);
bool parallelDir = llvm::omp::topParallelSet.test(dirContext.directive);
bool teamsDir = llvm::omp::allTeamsSet.test(dirContext.directive);
bool isStaticStorageDuration = IsSymbolStaticStorageDuration(*symbol);

Expand Down Expand Up @@ -2512,8 +2613,19 @@ void OmpAttributeVisitor::CreateImplicitSymbols(const Symbol *symbol) {
useLastDeclSymbol();
PRINT_IMPLICIT_RULE("3) enclosing context");
} else if (targetDir) {
// TODO 4) not mapped target variable -> firstprivate
// 4) not mapped target variable -> firstprivate
// - i.e. implicit, but meets OpenMP specification rules for
// firstprivate "promotion"
if (enableDelayedPrivatizationStaging &&
IsTargetCaptureImplicitlyFirstprivatizeable(*symbol, prevDSA,
dataSharingAttributeFlags, dataMappingAttributeFlags,
dirContext.defaultMap)) {
prevDSA.set(Symbol::Flag::OmpImplicit);
prevDSA.set(Symbol::Flag::OmpFirstPrivate);
makeSymbol(prevDSA);
}
dsa = prevDSA;
PRINT_IMPLICIT_RULE("4) not mapped target variable -> firstprivate");
} else if (taskGenDir) {
// TODO 5) dummy arg in orphaned taskgen construct -> firstprivate
if (prevDSA.test(Symbol::Flag::OmpShared) ||
Expand Down
Loading