Skip to content

[flang][OpenMP] Generalize isOpenMPPrivatizingConstruct #148654

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.

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

Open
wants to merge 1 commit into
base: users/kparzysz/e03-omp-get-directive
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
57 changes: 45 additions & 12 deletions flang/lib/Lower/OpenMP/DataSharingProcessor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@
#include "flang/Optimizer/HLFIR/HLFIROps.h"
#include "flang/Semantics/attr.h"
#include "flang/Semantics/tools.h"
#include "llvm/ADT/Sequence.h"
#include "llvm/ADT/SmallSet.h"

namespace Fortran {
namespace lower {
Expand All @@ -49,7 +51,7 @@ DataSharingProcessor::DataSharingProcessor(
firOpBuilder(converter.getFirOpBuilder()), clauses(clauses), eval(eval),
shouldCollectPreDeterminedSymbols(shouldCollectPreDeterminedSymbols),
useDelayedPrivatization(useDelayedPrivatization), symTable(symTable),
visitor() {
visitor(semaCtx) {
eval.visit([&](const auto &functionParserNode) {
parser::Walk(functionParserNode, visitor);
});
Expand Down Expand Up @@ -424,24 +426,55 @@ getSource(const semantics::SemanticsContext &semaCtx,
return source;
}

static void collectPrivatizingConstructs(
llvm::SmallSet<llvm::omp::Directive, 16> &constructs, unsigned version) {
using Clause = llvm::omp::Clause;
using Directive = llvm::omp::Directive;

static const Clause privatizingClauses[] = {
Clause::OMPC_private,
Clause::OMPC_lastprivate,
Clause::OMPC_firstprivate,
Clause::OMPC_in_reduction,
Clause::OMPC_reduction,
Clause::OMPC_linear,
// TODO: Clause::OMPC_induction,
Clause::OMPC_task_reduction,
Clause::OMPC_detach,
Clause::OMPC_use_device_ptr,
Clause::OMPC_is_device_ptr,
};

for (auto dir : llvm::enum_seq_inclusive<Directive>(Directive::First_,
Directive::Last_)) {
bool allowsPrivatizing = llvm::any_of(privatizingClauses, [&](Clause cls) {
return llvm::omp::isAllowedClauseForDirective(dir, cls, version);
});
if (allowsPrivatizing)
constructs.insert(dir);
}
}

bool DataSharingProcessor::isOpenMPPrivatizingConstruct(
const parser::OpenMPConstruct &omp) {
return common::visit(
[](auto &&s) {
using BareS = llvm::remove_cvref_t<decltype(s)>;
return std::is_same_v<BareS, parser::OpenMPBlockConstruct> ||
std::is_same_v<BareS, parser::OpenMPLoopConstruct> ||
std::is_same_v<BareS, parser::OpenMPSectionsConstruct>;
},
omp.u);
const parser::OpenMPConstruct &omp, unsigned version) {
static llvm::SmallSet<llvm::omp::Directive, 16> privatizing;
[[maybe_unused]] static bool init =
(collectPrivatizingConstructs(privatizing, version), true);

// As of OpenMP 6.0, privatizing constructs (with the test being if they
// allow a privatizing clause) are: dispatch, distribute, do, for, loop,
// parallel, scope, sections, simd, single, target, target_data, task,
// taskgroup, taskloop, and teams.
return llvm::is_contained(privatizing, extractOmpDirective(omp));
}

bool DataSharingProcessor::isOpenMPPrivatizingEvaluation(
const pft::Evaluation &eval) const {
return eval.visit([](auto &&s) {
unsigned version = semaCtx.langOptions().OpenMPVersion;
return eval.visit([=](auto &&s) {
using BareS = llvm::remove_cvref_t<decltype(s)>;
if constexpr (std::is_same_v<BareS, parser::OpenMPConstruct>) {
return isOpenMPPrivatizingConstruct(s);
return isOpenMPPrivatizingConstruct(s, version);
} else {
return false;
}
Expand Down
12 changes: 9 additions & 3 deletions flang/lib/Lower/OpenMP/DataSharingProcessor.h
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ class DataSharingProcessor {
/// at any point in time. This is used to track Symbol definition scopes in
/// order to tell which OMP scope defined vs. references a certain Symbol.
struct OMPConstructSymbolVisitor {
OMPConstructSymbolVisitor(semantics::SemanticsContext &ctx)
: version(ctx.langOptions().OpenMPVersion) {}
template <typename T>
bool Pre(const T &) {
return true;
Expand All @@ -45,13 +47,13 @@ class DataSharingProcessor {

bool Pre(const parser::OpenMPConstruct &omp) {
// Skip constructs that may not have privatizations.
if (isOpenMPPrivatizingConstruct(omp))
if (isOpenMPPrivatizingConstruct(omp, version))
constructs.push_back(&omp);
return true;
}

void Post(const parser::OpenMPConstruct &omp) {
if (isOpenMPPrivatizingConstruct(omp))
if (isOpenMPPrivatizingConstruct(omp, version))
constructs.pop_back();
}

Expand All @@ -68,6 +70,9 @@ class DataSharingProcessor {
/// construct that defines symbol.
bool isSymbolDefineBy(const semantics::Symbol *symbol,
lower::pft::Evaluation &eval) const;

private:
unsigned version;
};

mlir::OpBuilder::InsertPoint lastPrivIP;
Expand Down Expand Up @@ -115,7 +120,8 @@ class DataSharingProcessor {
mlir::OpBuilder::InsertPoint *lastPrivIP);
void insertDeallocs();

static bool isOpenMPPrivatizingConstruct(const parser::OpenMPConstruct &omp);
static bool isOpenMPPrivatizingConstruct(const parser::OpenMPConstruct &omp,
unsigned version);
bool isOpenMPPrivatizingEvaluation(const pft::Evaluation &eval) const;

public:
Expand Down
5 changes: 3 additions & 2 deletions flang/test/Lower/OpenMP/taskgroup02.f90
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,9 @@
! Check that variables are not privatized twice when TASKGROUP is used.

!CHECK-LABEL: func.func @_QPsub() {
!CHECK: omp.parallel {
!CHECK: %[[PAR_I:.*]]:2 = hlfir.declare %{{.*}} {uniq_name = "_QFsubEi"}
!CHECK: omp.parallel private(@_QFsubEi_private_i32 %[[SUB_I:.*]]#0 -> %[[ARG:.*]] : !fir.ref<i32>)
!CHECK: %[[ALLOCA:.*]] = fir.alloca i32
!CHECK: %[[PAR_I:.*]]:2 = hlfir.declare %[[ALLOCA]] {uniq_name = "_QFsubEi"}
Comment on lines -6 to +8
Copy link
Contributor

Choose a reason for hiding this comment

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

In this test, was the issue the fact that i was not being privatized because taskgroup was not being considered as a privatizing construct, which made i not be collected as a symbol in nested regions?

Was i getting a private copy just by accident, because it was a loop-iteration variable inside parallel in this case?

Copy link
Contributor Author

@kparzysz kparzysz Jul 14, 2025

Choose a reason for hiding this comment

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

We treated all block-associated constructs as privatizing, including master (which isn't one). When master is no longer considered as privatizing the result is that we privatize i in `parallel.

!CHECK: omp.master {
!CHECK: omp.taskgroup {
!CHECK-NEXT: omp.task private(@_QFsubEi_firstprivate_i32 %[[PAR_I]]#0 -> %[[TASK_I:.*]] : !fir.ref<i32>) {
Expand Down
4 changes: 4 additions & 0 deletions llvm/include/llvm/Frontend/OpenMP/OMP.h
Original file line number Diff line number Diff line change
Expand Up @@ -51,13 +51,17 @@ static constexpr inline bool canHaveIterator(Clause C) {
// Can clause C create a private copy of a variable.
static constexpr inline bool isPrivatizingClause(Clause C) {
switch (C) {
case OMPC_detach:
case OMPC_firstprivate:
// TODO case OMPC_induction:
case OMPC_in_reduction:
case OMPC_is_device_ptr:
case OMPC_lastprivate:
case OMPC_linear:
case OMPC_private:
case OMPC_reduction:
case OMPC_task_reduction:
case OMPC_use_device_ptr:
return true;
default:
return false;
Expand Down
Loading