[llvm-branch-commits] [flang] [flang][OpenMP][NFC] Refactor to avoid global variable (PR #144087)
ergawy wrote: I have no idea how I closed this! Sorry @tblah, can you please reopen it? https://github.com/llvm/llvm-project/pull/144087 ___ llvm-branch-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [flang] [flang][OpenMP][NFC] Refactor to avoid global variable (PR #144087)
ergawy wrote: Reopening since I closed it by mistake somehow. https://github.com/llvm/llvm-project/pull/144087 ___ llvm-branch-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [flang] [flang][OpenMP][NFC] Refactor to avoid global variable (PR #144087)
https://github.com/ergawy closed https://github.com/llvm/llvm-project/pull/144087 ___ llvm-branch-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [flang] [flang][OpenMP][NFC] Refactor to avoid global variable (PR #144087)
llvmbot wrote:
@llvm/pr-subscribers-flang-fir-hlfir
Author: Tom Eccles (tblah)
Changes
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 DumpAtomicAnalysis("fdebug-dump-atomic-analysis");
+namespace {
+struct OmpLoweringContext;
+} // namespace
+
//===--===//
// Code generation helper functions
//===--===//
@@ -55,6 +59,7 @@ static llvm::cl::opt
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 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;
-static llvm::SmallVector
-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;
+
+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 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 *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);
[llvm-branch-commits] [flang] [flang][OpenMP][NFC] Refactor to avoid global variable (PR #144087)
https://github.com/tblah created
https://github.com/llvm/llvm-project/pull/144087
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?
>From b962af9da5a74b2b5509f654299c3b9c35dca05d Mon Sep 17 00:00:00 2001
From: Tom Eccles
Date: Fri, 13 Jun 2025 14:58:56 +
Subject: [PATCH] [flang][OpenMP][NFC] Refactor to avoid global variable
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?
---
flang/lib/Lower/OpenMP/OpenMP.cpp | 560 +-
1 file changed, 310 insertions(+), 250 deletions(-)
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 DumpAtomicAnalysis("fdebug-dump-atomic-analysis");
+namespace {
+struct OmpLoweringContext;
+} // namespace
+
//===--===//
// Code generation helper functions
//===--===//
@@ -55,6 +59,7 @@ static llvm::cl::opt
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 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;
-static llvm::SmallVector
-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;
+
+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 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 *value) {
clauses = value;
@@ -1187,6 +1203,8 @@ struct OpWithBodyGenInfo {
lower::SymMap &symTable;
/// [in] Semantics context
semantic
