[llvm-branch-commits] [flang] [flang][OpenMP][NFC] Refactor to avoid global variable (PR #144087)
via llvm-branch-commits
llvm-branch-commits at lists.llvm.org
Fri Jun 13 08:04:47 PDT 2025
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-flang-fir-hlfir
Author: Tom Eccles (tblah)
<details>
<summary>Changes</summary>
Based on top of #<!-- -->144013
I was really hoping this would also work for `hostEvalInfo` but unfortunately that needed to be shared to a greater degree.
The same technique should work for that but it would need that class to be made public and then the state kept between calls to `genOpenMP*Construct`, which felt like more trouble than it was worth.
I'm open to abandoning this patch if solving one global variable doesn't feel worth this much churn.
Making these changes I was wondering if we should implement this file with one big class to wrap up all the state passed to every function. Any thoughts?
---
Patch is 75.76 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/144087.diff
1 Files Affected:
- (modified) flang/lib/Lower/OpenMP/OpenMP.cpp (+310-250)
``````````diff
diff --git a/flang/lib/Lower/OpenMP/OpenMP.cpp b/flang/lib/Lower/OpenMP/OpenMP.cpp
index 060eba1b906e3..9c0bfa95f8382 100644
--- a/flang/lib/Lower/OpenMP/OpenMP.cpp
+++ b/flang/lib/Lower/OpenMP/OpenMP.cpp
@@ -48,6 +48,10 @@ using namespace Fortran::common::openmp;
static llvm::cl::opt<bool> DumpAtomicAnalysis("fdebug-dump-atomic-analysis");
+namespace {
+struct OmpLoweringContext;
+} // namespace
+
//===----------------------------------------------------------------------===//
// Code generation helper functions
//===----------------------------------------------------------------------===//
@@ -55,6 +59,7 @@ static llvm::cl::opt<bool> DumpAtomicAnalysis("fdebug-dump-atomic-analysis");
static void genOMPDispatch(lower::AbstractConverter &converter,
lower::SymMap &symTable,
semantics::SemanticsContext &semaCtx,
+ OmpLoweringContext &ompCtx,
lower::pft::Evaluation &eval, mlir::Location loc,
const ConstructQueue &queue,
ConstructQueue::const_iterator item);
@@ -191,18 +196,28 @@ class HostEvalInfo {
llvm::SmallVector<const semantics::Symbol *> iv;
bool loopNestApplied = false, parallelApplied = false;
};
-} // namespace
/// Stack of \see HostEvalInfo to represent the current nest of \c omp.target
/// operations being created.
///
/// The current implementation prevents nested 'target' regions from breaking
/// the handling of the outer region by keeping a stack of information
-/// structures, but it will probably still require some further work to support
-/// reverse offloading.
-static llvm::SmallVector<HostEvalInfo, 0> hostEvalInfo;
-static llvm::SmallVector<const parser::OpenMPSectionsConstruct *, 0>
- sectionsStack;
+/// structures, but it will probably still require some further work to
+/// support reverse offloading.
+///
+/// This has to be a global rather than in OmpLoweringContext because different
+/// calls to void Fortran::lower::genOpenMPConstruct and
+/// Fortran::lower::genOpenMPDeclarativeConstruct need to share the same
+/// instance. FIXME: Maybe this should be promoted into the interface for those
+/// functions.
+llvm::SmallVector<HostEvalInfo, 0> hostEvalInfo;
+
+struct OmpLoweringContext {
+ /// Stack of parse tree information about the sections construct to allow each
+ /// section to be lowered as part of the enclosing sections construct.
+ llvm::SmallVector<const parser::OpenMPSectionsConstruct *, 0> sectionsStack;
+};
+} // namespace
/// Bind symbols to their corresponding entry block arguments.
///
@@ -1151,10 +1166,11 @@ struct OpWithBodyGenInfo {
OpWithBodyGenInfo(lower::AbstractConverter &converter,
lower::SymMap &symTable,
- semantics::SemanticsContext &semaCtx, mlir::Location loc,
+ semantics::SemanticsContext &semaCtx,
+ OmpLoweringContext &ompCtx, mlir::Location loc,
lower::pft::Evaluation &eval, llvm::omp::Directive dir)
- : converter(converter), symTable(symTable), semaCtx(semaCtx), loc(loc),
- eval(eval), dir(dir) {}
+ : converter(converter), symTable(symTable), semaCtx(semaCtx),
+ ompCtx(ompCtx), loc(loc), eval(eval), dir(dir) {}
OpWithBodyGenInfo &setClauses(const List<Clause> *value) {
clauses = value;
@@ -1187,6 +1203,8 @@ struct OpWithBodyGenInfo {
lower::SymMap &symTable;
/// [in] Semantics context
semantics::SemanticsContext &semaCtx;
+ /// [in] OpenMP context
+ OmpLoweringContext &ompCtx;
/// [in] location in source code.
mlir::Location loc;
/// [in] current PFT node/evaluation.
@@ -1290,8 +1308,8 @@ static void createBodyOfOp(mlir::Operation &op, const OpWithBodyGenInfo &info,
if (!info.genSkeletonOnly) {
if (ConstructQueue::const_iterator next = std::next(item);
next != queue.end()) {
- genOMPDispatch(info.converter, info.symTable, info.semaCtx, info.eval,
- info.loc, queue, next);
+ genOMPDispatch(info.converter, info.symTable, info.semaCtx, info.ompCtx,
+ info.eval, info.loc, queue, next);
} else {
// genFIR(Evaluation&) tries to patch up unterminated blocks, causing
// a lot of complications for our approach if the terminator generation
@@ -1383,10 +1401,10 @@ static void createBodyOfOp(mlir::Operation &op, const OpWithBodyGenInfo &info,
static void genBodyOfTargetDataOp(
lower::AbstractConverter &converter, lower::SymMap &symTable,
- semantics::SemanticsContext &semaCtx, lower::pft::Evaluation &eval,
- mlir::omp::TargetDataOp &dataOp, const EntryBlockArgs &args,
- const mlir::Location ¤tLocation, const ConstructQueue &queue,
- ConstructQueue::const_iterator item) {
+ semantics::SemanticsContext &semaCtx, OmpLoweringContext &ompCtx,
+ lower::pft::Evaluation &eval, mlir::omp::TargetDataOp &dataOp,
+ const EntryBlockArgs &args, const mlir::Location ¤tLocation,
+ const ConstructQueue &queue, ConstructQueue::const_iterator item) {
fir::FirOpBuilder &firOpBuilder = converter.getFirOpBuilder();
genEntryBlock(firOpBuilder, args, dataOp.getRegion());
@@ -1414,8 +1432,8 @@ static void genBodyOfTargetDataOp(
if (ConstructQueue::const_iterator next = std::next(item);
next != queue.end()) {
- genOMPDispatch(converter, symTable, semaCtx, eval, currentLocation, queue,
- next);
+ genOMPDispatch(converter, symTable, semaCtx, ompCtx, eval, currentLocation,
+ queue, next);
} else {
genNestedEvaluations(converter, eval);
}
@@ -1458,10 +1476,11 @@ static void genIntermediateCommonBlockAccessors(
// all the symbols present in mapSymbols as block arguments to this block.
static void genBodyOfTargetOp(
lower::AbstractConverter &converter, lower::SymMap &symTable,
- semantics::SemanticsContext &semaCtx, lower::pft::Evaluation &eval,
- mlir::omp::TargetOp &targetOp, const EntryBlockArgs &args,
- const mlir::Location ¤tLocation, const ConstructQueue &queue,
- ConstructQueue::const_iterator item, DataSharingProcessor &dsp) {
+ semantics::SemanticsContext &semaCtx, OmpLoweringContext &ompCtx,
+ lower::pft::Evaluation &eval, mlir::omp::TargetOp &targetOp,
+ const EntryBlockArgs &args, const mlir::Location ¤tLocation,
+ const ConstructQueue &queue, ConstructQueue::const_iterator item,
+ DataSharingProcessor &dsp) {
fir::FirOpBuilder &firOpBuilder = converter.getFirOpBuilder();
auto argIface = llvm::cast<mlir::omp::BlockArgOpenMPOpInterface>(*targetOp);
@@ -1606,8 +1625,8 @@ static void genBodyOfTargetOp(
if (ConstructQueue::const_iterator next = std::next(item);
next != queue.end()) {
- genOMPDispatch(converter, symTable, semaCtx, eval, currentLocation, queue,
- next);
+ genOMPDispatch(converter, symTable, semaCtx, ompCtx, eval, currentLocation,
+ queue, next);
} else {
genNestedEvaluations(converter, eval);
}
@@ -1703,8 +1722,9 @@ static void genFlushClauses(lower::AbstractConverter &converter,
static void
genLoopNestClauses(lower::AbstractConverter &converter,
semantics::SemanticsContext &semaCtx,
- lower::pft::Evaluation &eval, const List<Clause> &clauses,
- mlir::Location loc, mlir::omp::LoopNestOperands &clauseOps,
+ OmpLoweringContext &ompCtx, lower::pft::Evaluation &eval,
+ const List<Clause> &clauses, mlir::Location loc,
+ mlir::omp::LoopNestOperands &clauseOps,
llvm::SmallVectorImpl<const semantics::Symbol *> &iv) {
ClauseProcessor cp(converter, semaCtx, clauses);
@@ -1746,8 +1766,9 @@ genOrderedRegionClauses(lower::AbstractConverter &converter,
static void genParallelClauses(
lower::AbstractConverter &converter, semantics::SemanticsContext &semaCtx,
- lower::StatementContext &stmtCtx, const List<Clause> &clauses,
- mlir::Location loc, mlir::omp::ParallelOperands &clauseOps,
+ lower::StatementContext &stmtCtx, OmpLoweringContext &ompCtx,
+ const List<Clause> &clauses, mlir::Location loc,
+ mlir::omp::ParallelOperands &clauseOps,
llvm::SmallVectorImpl<const semantics::Symbol *> &reductionSyms) {
ClauseProcessor cp(converter, semaCtx, clauses);
cp.processAllocate(clauseOps);
@@ -1812,9 +1833,9 @@ static void genSingleClauses(lower::AbstractConverter &converter,
static void genTargetClauses(
lower::AbstractConverter &converter, semantics::SemanticsContext &semaCtx,
lower::SymMap &symTable, lower::StatementContext &stmtCtx,
- lower::pft::Evaluation &eval, const List<Clause> &clauses,
- mlir::Location loc, mlir::omp::TargetOperands &clauseOps,
- DefaultMapsTy &defaultMaps,
+ OmpLoweringContext &ompCtx, lower::pft::Evaluation &eval,
+ const List<Clause> &clauses, mlir::Location loc,
+ mlir::omp::TargetOperands &clauseOps, DefaultMapsTy &defaultMaps,
llvm::SmallVectorImpl<const semantics::Symbol *> &hasDeviceAddrSyms,
llvm::SmallVectorImpl<const semantics::Symbol *> &isDevicePtrSyms,
llvm::SmallVectorImpl<const semantics::Symbol *> &mapSyms) {
@@ -1956,8 +1977,9 @@ static void genWorkshareClauses(lower::AbstractConverter &converter,
static void genTeamsClauses(
lower::AbstractConverter &converter, semantics::SemanticsContext &semaCtx,
- lower::StatementContext &stmtCtx, const List<Clause> &clauses,
- mlir::Location loc, mlir::omp::TeamsOperands &clauseOps,
+ lower::StatementContext &stmtCtx, OmpLoweringContext &ompCtx,
+ const List<Clause> &clauses, mlir::Location loc,
+ mlir::omp::TeamsOperands &clauseOps,
llvm::SmallVectorImpl<const semantics::Symbol *> &reductionSyms) {
ClauseProcessor cp(converter, semaCtx, clauses);
cp.processAllocate(clauseOps);
@@ -2027,7 +2049,7 @@ static mlir::omp::CancellationPointOp genCancellationPointOp(
static mlir::omp::CriticalOp
genCriticalOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
- semantics::SemanticsContext &semaCtx,
+ semantics::SemanticsContext &semaCtx, OmpLoweringContext &ompCtx,
lower::pft::Evaluation &eval, mlir::Location loc,
const ConstructQueue &queue, ConstructQueue::const_iterator item,
const std::optional<parser::Name> &name) {
@@ -2051,7 +2073,7 @@ genCriticalOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
}
return genOpWithBody<mlir::omp::CriticalOp>(
- OpWithBodyGenInfo(converter, symTable, semaCtx, loc, eval,
+ OpWithBodyGenInfo(converter, symTable, semaCtx, ompCtx, loc, eval,
llvm::omp::Directive::OMPD_critical),
queue, item, nameAttr);
}
@@ -2071,9 +2093,10 @@ genFlushOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
static mlir::omp::LoopNestOp genLoopNestOp(
lower::AbstractConverter &converter, lower::SymMap &symTable,
- semantics::SemanticsContext &semaCtx, lower::pft::Evaluation &eval,
- mlir::Location loc, const ConstructQueue &queue,
- ConstructQueue::const_iterator item, mlir::omp::LoopNestOperands &clauseOps,
+ semantics::SemanticsContext &semaCtx, OmpLoweringContext &ompCtx,
+ lower::pft::Evaluation &eval, mlir::Location loc,
+ const ConstructQueue &queue, ConstructQueue::const_iterator item,
+ mlir::omp::LoopNestOperands &clauseOps,
llvm::ArrayRef<const semantics::Symbol *> iv,
llvm::ArrayRef<
std::pair<mlir::omp::BlockArgOpenMPOpInterface, const EntryBlockArgs &>>
@@ -2088,7 +2111,7 @@ static mlir::omp::LoopNestOp genLoopNestOp(
getCollapsedLoopEval(eval, getCollapseValue(item->clauses));
return genOpWithBody<mlir::omp::LoopNestOp>(
- OpWithBodyGenInfo(converter, symTable, semaCtx, loc, *nestedEval,
+ OpWithBodyGenInfo(converter, symTable, semaCtx, ompCtx, loc, *nestedEval,
directive)
.setClauses(&item->clauses)
.setDataSharingProcessor(&dsp)
@@ -2098,9 +2121,9 @@ static mlir::omp::LoopNestOp genLoopNestOp(
static mlir::omp::LoopOp
genLoopOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
- semantics::SemanticsContext &semaCtx, lower::pft::Evaluation &eval,
- mlir::Location loc, const ConstructQueue &queue,
- ConstructQueue::const_iterator item) {
+ semantics::SemanticsContext &semaCtx, OmpLoweringContext &ompCtx,
+ lower::pft::Evaluation &eval, mlir::Location loc,
+ const ConstructQueue &queue, ConstructQueue::const_iterator item) {
mlir::omp::LoopOperands loopClauseOps;
llvm::SmallVector<const semantics::Symbol *> loopReductionSyms;
genLoopClauses(converter, semaCtx, item->clauses, loc, loopClauseOps,
@@ -2113,7 +2136,7 @@ genLoopOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
mlir::omp::LoopNestOperands loopNestClauseOps;
llvm::SmallVector<const semantics::Symbol *> iv;
- genLoopNestClauses(converter, semaCtx, eval, item->clauses, loc,
+ genLoopNestClauses(converter, semaCtx, ompCtx, eval, item->clauses, loc,
loopNestClauseOps, iv);
EntryBlockArgs loopArgs;
@@ -2124,7 +2147,7 @@ genLoopOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
auto loopOp =
genWrapperOp<mlir::omp::LoopOp>(converter, loc, loopClauseOps, loopArgs);
- genLoopNestOp(converter, symTable, semaCtx, eval, loc, queue, item,
+ genLoopNestOp(converter, symTable, semaCtx, ompCtx, eval, loc, queue, item,
loopNestClauseOps, iv, {{loopOp, loopArgs}},
llvm::omp::Directive::OMPD_loop, dsp);
return loopOp;
@@ -2133,25 +2156,25 @@ genLoopOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
static mlir::omp::MaskedOp
genMaskedOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
lower::StatementContext &stmtCtx,
- semantics::SemanticsContext &semaCtx, lower::pft::Evaluation &eval,
- mlir::Location loc, const ConstructQueue &queue,
- ConstructQueue::const_iterator item) {
+ semantics::SemanticsContext &semaCtx, OmpLoweringContext &ompCtx,
+ lower::pft::Evaluation &eval, mlir::Location loc,
+ const ConstructQueue &queue, ConstructQueue::const_iterator item) {
mlir::omp::MaskedOperands clauseOps;
genMaskedClauses(converter, semaCtx, stmtCtx, item->clauses, loc, clauseOps);
return genOpWithBody<mlir::omp::MaskedOp>(
- OpWithBodyGenInfo(converter, symTable, semaCtx, loc, eval,
+ OpWithBodyGenInfo(converter, symTable, semaCtx, ompCtx, loc, eval,
llvm::omp::Directive::OMPD_masked),
queue, item, clauseOps);
}
static mlir::omp::MasterOp
genMasterOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
- semantics::SemanticsContext &semaCtx, lower::pft::Evaluation &eval,
- mlir::Location loc, const ConstructQueue &queue,
- ConstructQueue::const_iterator item) {
+ semantics::SemanticsContext &semaCtx, OmpLoweringContext &ompCtx,
+ lower::pft::Evaluation &eval, mlir::Location loc,
+ const ConstructQueue &queue, ConstructQueue::const_iterator item) {
return genOpWithBody<mlir::omp::MasterOp>(
- OpWithBodyGenInfo(converter, symTable, semaCtx, loc, eval,
+ OpWithBodyGenInfo(converter, symTable, semaCtx, ompCtx, loc, eval,
llvm::omp::Directive::OMPD_master),
queue, item);
}
@@ -2168,21 +2191,21 @@ genOrderedOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
static mlir::omp::OrderedRegionOp
genOrderedRegionOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
semantics::SemanticsContext &semaCtx,
- lower::pft::Evaluation &eval, mlir::Location loc,
- const ConstructQueue &queue,
+ OmpLoweringContext &ompCtx, lower::pft::Evaluation &eval,
+ mlir::Location loc, const ConstructQueue &queue,
ConstructQueue::const_iterator item) {
mlir::omp::OrderedRegionOperands clauseOps;
genOrderedRegionClauses(converter, semaCtx, item->clauses, loc, clauseOps);
return genOpWithBody<mlir::omp::OrderedRegionOp>(
- OpWithBodyGenInfo(converter, symTable, semaCtx, loc, eval,
+ OpWithBodyGenInfo(converter, symTable, semaCtx, ompCtx, loc, eval,
llvm::omp::Directive::OMPD_ordered),
queue, item, clauseOps);
}
static mlir::omp::ParallelOp
genParallelOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
- semantics::SemanticsContext &semaCtx,
+ semantics::SemanticsContext &semaCtx, OmpLoweringContext &ompCtx,
lower::pft::Evaluation &eval, mlir::Location loc,
const ConstructQueue &queue, ConstructQueue::const_iterator item,
mlir::omp::ParallelOperands &clauseOps,
@@ -2192,7 +2215,7 @@ genParallelOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
"expected valid DataSharingProcessor");
OpWithBodyGenInfo genInfo =
- OpWithBodyGenInfo(converter, symTable, semaCtx, loc, eval,
+ OpWithBodyGenInfo(converter, symTable, semaCtx, ompCtx, loc, eval,
llvm::omp::Directive::OMPD_parallel)
.setClauses(&item->clauses)
.setEntryBlockArgs(&args)
@@ -2220,14 +2243,14 @@ genScanOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
/// lowered here with correct reduction symbol remapping.
static mlir::omp::SectionsOp
genSectionsOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
- semantics::SemanticsContext &semaCtx,
+ semantics::SemanticsContext &semaCtx, OmpLoweringContext &ompCtx,
lower::pft::Evaluation &eval, mlir::Location loc,
const ConstructQueue &queue,
ConstructQueue::const_iterator item) {
- assert(!sectionsStack.empty());
+ assert(!ompCtx.sectionsStack.empty());
const auto §ionBlocks =
- std::get<parser::OmpSectionBlocks>(sectionsStack.back()->t);
- sectionsStack.pop_back();
+ std::get<parser::OmpSectionBlocks>(ompCtx.sectionsStack.back()->t);
+ ompCtx.sectionsStack.pop_back();
mlir::omp::SectionsOperands clauseOps;
llvm::SmallVector<const semantics::Symbol *> reductionSyms;
genSectionsClauses(converter, semaCtx, item->clauses, loc, clauseOps,
@@ -2295,7 +2318,7 @@ genSectionsOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
builder.setInsertionPoint(terminator);
genOpWithBody<mlir::omp::SectionOp>(
- OpWithBodyGenInfo(converter, symTable, semaCtx, loc, nestedEval,
+ OpWithBodyGenInfo(converter, symTable, semaCtx, ompCtx, loc, nestedEval,
llvm::omp::Directive::OMPD_section)
.setClauses(§ionQueue.begin()->clauses)
.setDataSharingProcessor(&dsp)
@@ -2355,14 +2378,14 @@ genScopeOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
static mlir::omp::SingleOp
genSingleOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
- semantics::SemanticsContext &semaCtx, lower::pft::Evaluation &eval,
- mlir::Location loc, const ConstructQueue &queue,
- ConstructQueue::const_iterator item) {
+ semantics::SemanticsContext &semaCtx, OmpLoweringContext &ompCtx,
+ lower::pft::Evaluation &eval, mlir::Location loc,
+ const ConstructQueue &queue, ConstructQueue::const_iterator item) {
mlir::omp::SingleOperands clauseOps;
genSingleClauses(converter, semaCtx, item->clauses, loc, clauseOps);
return genOpWithBody<mlir::omp::SingleOp>(
- OpWithBodyGenInfo(converter, symTable, semaCtx, loc, eval,
+ OpWithBodyGenInfo(converter, symTable, semaCtx, ompCtx, loc, eval,
llvm::omp::Directive::OMPD_single)
.setClauses(&item->clauses),
queue, item, clauseOps);
@@ -2371,9 +2394,9 @@ genSingleOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
static mlir::omp::TargetOp
genTargetOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
lower::StatementContext &stmtCtx,
- semantic...
[truncated]
``````````
</details>
https://github.com/llvm/llvm-project/pull/144087
More information about the llvm-branch-commits
mailing list