[flang][OpenMP][NFC] Refactor to avoid global variable #144087
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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?
llvmbot
added
flang
|
@llvm/pr-subscribers-flang-fir-hlfir Author: Tom Eccles (tblah) ChangesBased on top of #144013 I was really hoping this would also work for 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 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:
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]
|
|
I have no idea how I closed this! Sorry @tblah. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Based on top of #144013
I was really hoping this would also work for
hostEvalInfobut 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?