[flang][OpenMP] Organize genOMP functions in OpenMP.cpp, NFC
#86309
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
Put all of the genOMP functions together, organize them in two groups:
for declarative constructs and for other (executable) constructs.
Replace visit functions for OpenMPDeclarativeConstruct and
OpenMPConstruct from listing individual visitors for each variant
alternative to using a single generic visitor. Essentially, going from
```
std::visit(
[](foo x) { genOMP(foo); }
[](bar x) { TODO }
[](baz x) { genOMP(baz); }
)
```
to
```
void genOMP(bar x) { // Separate visitor for an unhandled case
TODO
}
[...]
std::visit([&](auto &&s) { genOMP(s); }) // generic
```
This doesn't change any functionality, just reorganizes the functions
a bit. The intent here is to improve the readability of this file.
llvmbot
added
flang
|
@llvm/pr-subscribers-flang-fir-hlfir Author: Krzysztof Parzyszek (kparzysz) ChangesPut all of the genOMP functions together, organize them in two groups: for declarative constructs and for other (executable) constructs. Replace visit functions for OpenMPDeclarativeConstruct and OpenMPConstruct from listing individual visitors for each variant alternative to using a single generic visitor. Essentially, going from to This doesn't change any functionality, just reorganizes the functions a bit. The intent here is to improve the readability of this file. Patch is 26.94 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/86309.diff 1 Files Affected:
diff --git a/flang/lib/Lower/OpenMP/OpenMP.cpp b/flang/lib/Lower/OpenMP/OpenMP.cpp
index d91694c4f639a5..033c3a9f80838e 100644
--- a/flang/lib/Lower/OpenMP/OpenMP.cpp
+++ b/flang/lib/Lower/OpenMP/OpenMP.cpp
@@ -1406,34 +1406,6 @@ genOmpFlush(Fortran::lower::AbstractConverter &converter,
converter.getCurrentLocation(), operandRange);
}
-static void
-genOMP(Fortran::lower::AbstractConverter &converter,
- Fortran::lower::SymMap &symTable,
- Fortran::semantics::SemanticsContext &semaCtx,
- Fortran::lower::pft::Evaluation &eval,
- const Fortran::parser::OpenMPStandaloneConstruct &standaloneConstruct) {
- std::visit(
- Fortran::common::visitors{
- [&](const Fortran::parser::OpenMPSimpleStandaloneConstruct
- &simpleStandaloneConstruct) {
- genOmpSimpleStandalone(converter, semaCtx, eval,
- /*genNested=*/true,
- simpleStandaloneConstruct);
- },
- [&](const Fortran::parser::OpenMPFlushConstruct &flushConstruct) {
- genOmpFlush(converter, semaCtx, eval, flushConstruct);
- },
- [&](const Fortran::parser::OpenMPCancelConstruct &cancelConstruct) {
- TODO(converter.getCurrentLocation(), "OpenMPCancelConstruct");
- },
- [&](const Fortran::parser::OpenMPCancellationPointConstruct
- &cancellationPointConstruct) {
- TODO(converter.getCurrentLocation(), "OpenMPCancelConstruct");
- },
- },
- standaloneConstruct.u);
-}
-
static llvm::SmallVector<const Fortran::semantics::Symbol *>
genLoopVars(mlir::Operation *op, Fortran::lower::AbstractConverter &converter,
mlir::Location &loc,
@@ -1672,88 +1644,177 @@ static void createSimdWsloop(
endClauseList, loc);
}
+static void
+markDeclareTarget(mlir::Operation *op,
+ Fortran::lower::AbstractConverter &converter,
+ mlir::omp::DeclareTargetCaptureClause captureClause,
+ mlir::omp::DeclareTargetDeviceType deviceType) {
+ // TODO: Add support for program local variables with declare target applied
+ auto declareTargetOp = llvm::dyn_cast<mlir::omp::DeclareTargetInterface>(op);
+ if (!declareTargetOp)
+ fir::emitFatalError(
+ converter.getCurrentLocation(),
+ "Attempt to apply declare target on unsupported operation");
+
+ // The function or global already has a declare target applied to it, very
+ // likely through implicit capture (usage in another declare target
+ // function/subroutine). It should be marked as any if it has been assigned
+ // both host and nohost, else we skip, as there is no change
+ if (declareTargetOp.isDeclareTarget()) {
+ if (declareTargetOp.getDeclareTargetDeviceType() != deviceType)
+ declareTargetOp.setDeclareTarget(mlir::omp::DeclareTargetDeviceType::any,
+ captureClause);
+ return;
+ }
+
+ declareTargetOp.setDeclareTarget(deviceType, captureClause);
+}
+
+// OpenMPDeclarativeConstruct visitors --------------------------------
+
+static void
+genOMP(Fortran::lower::AbstractConverter &converter,
+ Fortran::lower::SymMap &symTable,
+ Fortran::semantics::SemanticsContext &semaCtx,
+ Fortran::lower::pft::Evaluation &eval,
+ const Fortran::parser::OpenMPDeclarativeAllocate &declarativeAllocate) {
+ TODO(converter.getCurrentLocation(), "OpenMPDeclarativeAllocate");
+}
+
static void genOMP(Fortran::lower::AbstractConverter &converter,
Fortran::lower::SymMap &symTable,
Fortran::semantics::SemanticsContext &semaCtx,
Fortran::lower::pft::Evaluation &eval,
- const Fortran::parser::OpenMPLoopConstruct &loopConstruct) {
- const auto &beginLoopDirective =
- std::get<Fortran::parser::OmpBeginLoopDirective>(loopConstruct.t);
- const auto &loopOpClauseList =
- std::get<Fortran::parser::OmpClauseList>(beginLoopDirective.t);
- mlir::Location currentLocation =
- converter.genLocation(beginLoopDirective.source);
- const auto ompDirective =
- std::get<Fortran::parser::OmpLoopDirective>(beginLoopDirective.t).v;
+ const Fortran::parser::OpenMPDeclareReductionConstruct
+ &declareReductionConstruct) {
+ TODO(converter.getCurrentLocation(), "OpenMPDeclareReductionConstruct");
+}
- const auto *endClauseList = [&]() {
- using RetTy = const Fortran::parser::OmpClauseList *;
- if (auto &endLoopDirective =
- std::get<std::optional<Fortran::parser::OmpEndLoopDirective>>(
- loopConstruct.t)) {
- return RetTy(
- &std::get<Fortran::parser::OmpClauseList>((*endLoopDirective).t));
- }
- return RetTy();
- }();
+static void genOMP(
+ Fortran::lower::AbstractConverter &converter,
+ Fortran::lower::SymMap &symTable,
+ Fortran::semantics::SemanticsContext &semaCtx,
+ Fortran::lower::pft::Evaluation &eval,
+ const Fortran::parser::OpenMPDeclareSimdConstruct &declareSimdConstruct) {
+ TODO(converter.getCurrentLocation(), "OpenMPDeclareSimdConstruct");
+}
- bool validDirective = false;
- if (llvm::omp::topTaskloopSet.test(ompDirective)) {
- validDirective = true;
- TODO(currentLocation, "Taskloop construct");
- } else {
- // Create omp.{target, teams, distribute, parallel} nested operations
- if ((llvm::omp::allTargetSet & llvm::omp::loopConstructSet)
- .test(ompDirective)) {
- validDirective = true;
- genTargetOp(converter, semaCtx, eval, /*genNested=*/false,
- currentLocation, loopOpClauseList, ompDirective,
- /*outerCombined=*/true);
- }
- if ((llvm::omp::allTeamsSet & llvm::omp::loopConstructSet)
- .test(ompDirective)) {
- validDirective = true;
- genTeamsOp(converter, semaCtx, eval, /*genNested=*/false, currentLocation,
- loopOpClauseList,
- /*outerCombined=*/true);
- }
- if (llvm::omp::allDistributeSet.test(ompDirective)) {
- validDirective = true;
- TODO(currentLocation, "Distribute construct");
- }
- if ((llvm::omp::allParallelSet & llvm::omp::loopConstructSet)
- .test(ompDirective)) {
- validDirective = true;
- genParallelOp(converter, symTable, semaCtx, eval, /*genNested=*/false,
- currentLocation, loopOpClauseList,
- /*outerCombined=*/true);
- }
- }
- if ((llvm::omp::allDoSet | llvm::omp::allSimdSet).test(ompDirective))
- validDirective = true;
+static void genOMP(Fortran::lower::AbstractConverter &converter,
+ Fortran::lower::SymMap &symTable,
+ Fortran::semantics::SemanticsContext &semaCtx,
+ Fortran::lower::pft::Evaluation &eval,
+ const Fortran::parser::OpenMPDeclareTargetConstruct
+ &declareTargetConstruct) {
+ llvm::SmallVector<DeclareTargetCapturePair> symbolAndClause;
+ mlir::ModuleOp mod = converter.getFirOpBuilder().getModule();
+ mlir::omp::DeclareTargetDeviceType deviceType = getDeclareTargetInfo(
+ converter, semaCtx, eval, declareTargetConstruct, symbolAndClause);
- if (!validDirective) {
- TODO(currentLocation, "Unhandled loop directive (" +
- llvm::omp::getOpenMPDirectiveName(ompDirective) +
- ")");
- }
+ for (const DeclareTargetCapturePair &symClause : symbolAndClause) {
+ mlir::Operation *op = mod.lookupSymbol(converter.mangleName(
+ std::get<const Fortran::semantics::Symbol &>(symClause)));
- if (llvm::omp::allDoSimdSet.test(ompDirective)) {
- // 2.9.3.2 Workshare SIMD construct
- createSimdWsloop(converter, semaCtx, eval, ompDirective, loopOpClauseList,
- endClauseList, currentLocation);
+ // Some symbols are deferred until later in the module, these are handled
+ // upon finalization of the module for OpenMP inside of Bridge, so we simply
+ // skip for now.
+ if (!op)
+ continue;
- } else if (llvm::omp::allSimdSet.test(ompDirective)) {
- // 2.9.3.1 SIMD construct
- createSimdLoop(converter, semaCtx, eval, ompDirective, loopOpClauseList,
- currentLocation);
- genOpenMPReduction(converter, semaCtx, loopOpClauseList);
- } else {
- createWsloop(converter, semaCtx, eval, ompDirective, loopOpClauseList,
- endClauseList, currentLocation);
+ markDeclareTarget(
+ op, converter,
+ std::get<mlir::omp::DeclareTargetCaptureClause>(symClause), deviceType);
}
}
+static void
+genOMP(Fortran::lower::AbstractConverter &converter,
+ Fortran::lower::SymMap &symTable,
+ Fortran::semantics::SemanticsContext &semaCtx,
+ Fortran::lower::pft::Evaluation &eval,
+ const Fortran::parser::OpenMPRequiresConstruct &requiresConstruct) {
+ // Requires directives are gathered and processed in semantics and
+ // then combined in the lowering bridge before triggering codegen
+ // just once. Hence, there is no need to lower each individual
+ // occurrence here.
+}
+
+static void genOMP(Fortran::lower::AbstractConverter &converter,
+ Fortran::lower::SymMap &symTable,
+ Fortran::semantics::SemanticsContext &semaCtx,
+ Fortran::lower::pft::Evaluation &eval,
+ const Fortran::parser::OpenMPThreadprivate &threadprivate) {
+ // The directive is lowered when instantiating the variable to
+ // support the case of threadprivate variable declared in module.
+}
+
+static void
+genOMP(Fortran::lower::AbstractConverter &converter,
+ Fortran::lower::SymMap &symTable,
+ Fortran::semantics::SemanticsContext &semaCtx,
+ Fortran::lower::pft::Evaluation &eval,
+ const Fortran::parser::OpenMPDeclarativeConstruct &ompDeclConstruct) {
+ std::visit(
+ [&](auto &&s) { return genOMP(converter, symTable, semaCtx, eval, s); },
+ ompDeclConstruct.u);
+}
+
+// OpenMPConstruct visitors -------------------------------------------
+
+static void
+genOMP(Fortran::lower::AbstractConverter &converter,
+ Fortran::lower::SymMap &symTable,
+ Fortran::semantics::SemanticsContext &semaCtx,
+ Fortran::lower::pft::Evaluation &eval,
+ const Fortran::parser::OpenMPAllocatorsConstruct &allocsConstruct) {
+ TODO(converter.getCurrentLocation(), "OpenMPAllocatorsConstruct");
+}
+
+static void
+genOMP(Fortran::lower::AbstractConverter &converter,
+ Fortran::lower::SymMap &symTable,
+ Fortran::semantics::SemanticsContext &semaCtx,
+ Fortran::lower::pft::Evaluation &eval,
+ const Fortran::parser::OpenMPAtomicConstruct &atomicConstruct) {
+ std::visit(
+ Fortran::common::visitors{
+ [&](const Fortran::parser::OmpAtomicRead &atomicRead) {
+ mlir::Location loc = converter.genLocation(atomicRead.source);
+ Fortran::lower::genOmpAccAtomicRead<
+ Fortran::parser::OmpAtomicRead,
+ Fortran::parser::OmpAtomicClauseList>(converter, atomicRead,
+ loc);
+ },
+ [&](const Fortran::parser::OmpAtomicWrite &atomicWrite) {
+ mlir::Location loc = converter.genLocation(atomicWrite.source);
+ Fortran::lower::genOmpAccAtomicWrite<
+ Fortran::parser::OmpAtomicWrite,
+ Fortran::parser::OmpAtomicClauseList>(converter, atomicWrite,
+ loc);
+ },
+ [&](const Fortran::parser::OmpAtomic &atomicConstruct) {
+ mlir::Location loc = converter.genLocation(atomicConstruct.source);
+ Fortran::lower::genOmpAtomic<Fortran::parser::OmpAtomic,
+ Fortran::parser::OmpAtomicClauseList>(
+ converter, atomicConstruct, loc);
+ },
+ [&](const Fortran::parser::OmpAtomicUpdate &atomicUpdate) {
+ mlir::Location loc = converter.genLocation(atomicUpdate.source);
+ Fortran::lower::genOmpAccAtomicUpdate<
+ Fortran::parser::OmpAtomicUpdate,
+ Fortran::parser::OmpAtomicClauseList>(converter, atomicUpdate,
+ loc);
+ },
+ [&](const Fortran::parser::OmpAtomicCapture &atomicCapture) {
+ mlir::Location loc = converter.genLocation(atomicCapture.source);
+ Fortran::lower::genOmpAccAtomicCapture<
+ Fortran::parser::OmpAtomicCapture,
+ Fortran::parser::OmpAtomicClauseList>(converter, atomicCapture,
+ loc);
+ },
+ },
+ atomicConstruct.u);
+}
+
static void
genOMP(Fortran::lower::AbstractConverter &converter,
Fortran::lower::SymMap &symTable,
@@ -1934,6 +1995,107 @@ genOMP(Fortran::lower::AbstractConverter &converter,
createBodyOfOp<mlir::omp::CriticalOp>(criticalOp, genInfo);
}
+static void
+genOMP(Fortran::lower::AbstractConverter &converter,
+ Fortran::lower::SymMap &symTable,
+ Fortran::semantics::SemanticsContext &semaCtx,
+ Fortran::lower::pft::Evaluation &eval,
+ const Fortran::parser::OpenMPExecutableAllocate &execAllocConstruct) {
+ TODO(converter.getCurrentLocation(), "OpenMPExecutableAllocate");
+}
+
+static void genOMP(Fortran::lower::AbstractConverter &converter,
+ Fortran::lower::SymMap &symTable,
+ Fortran::semantics::SemanticsContext &semaCtx,
+ Fortran::lower::pft::Evaluation &eval,
+ const Fortran::parser::OpenMPLoopConstruct &loopConstruct) {
+ const auto &beginLoopDirective =
+ std::get<Fortran::parser::OmpBeginLoopDirective>(loopConstruct.t);
+ const auto &loopOpClauseList =
+ std::get<Fortran::parser::OmpClauseList>(beginLoopDirective.t);
+ mlir::Location currentLocation =
+ converter.genLocation(beginLoopDirective.source);
+ const auto ompDirective =
+ std::get<Fortran::parser::OmpLoopDirective>(beginLoopDirective.t).v;
+
+ const auto *endClauseList = [&]() {
+ using RetTy = const Fortran::parser::OmpClauseList *;
+ if (auto &endLoopDirective =
+ std::get<std::optional<Fortran::parser::OmpEndLoopDirective>>(
+ loopConstruct.t)) {
+ return RetTy(
+ &std::get<Fortran::parser::OmpClauseList>((*endLoopDirective).t));
+ }
+ return RetTy();
+ }();
+
+ bool validDirective = false;
+ if (llvm::omp::topTaskloopSet.test(ompDirective)) {
+ validDirective = true;
+ TODO(currentLocation, "Taskloop construct");
+ } else {
+ // Create omp.{target, teams, distribute, parallel} nested operations
+ if ((llvm::omp::allTargetSet & llvm::omp::loopConstructSet)
+ .test(ompDirective)) {
+ validDirective = true;
+ genTargetOp(converter, semaCtx, eval, /*genNested=*/false,
+ currentLocation, loopOpClauseList, ompDirective,
+ /*outerCombined=*/true);
+ }
+ if ((llvm::omp::allTeamsSet & llvm::omp::loopConstructSet)
+ .test(ompDirective)) {
+ validDirective = true;
+ genTeamsOp(converter, semaCtx, eval, /*genNested=*/false, currentLocation,
+ loopOpClauseList,
+ /*outerCombined=*/true);
+ }
+ if (llvm::omp::allDistributeSet.test(ompDirective)) {
+ validDirective = true;
+ TODO(currentLocation, "Distribute construct");
+ }
+ if ((llvm::omp::allParallelSet & llvm::omp::loopConstructSet)
+ .test(ompDirective)) {
+ validDirective = true;
+ genParallelOp(converter, symTable, semaCtx, eval, /*genNested=*/false,
+ currentLocation, loopOpClauseList,
+ /*outerCombined=*/true);
+ }
+ }
+ if ((llvm::omp::allDoSet | llvm::omp::allSimdSet).test(ompDirective))
+ validDirective = true;
+
+ if (!validDirective) {
+ TODO(currentLocation, "Unhandled loop directive (" +
+ llvm::omp::getOpenMPDirectiveName(ompDirective) +
+ ")");
+ }
+
+ if (llvm::omp::allDoSimdSet.test(ompDirective)) {
+ // 2.9.3.2 Workshare SIMD construct
+ createSimdWsloop(converter, semaCtx, eval, ompDirective, loopOpClauseList,
+ endClauseList, currentLocation);
+
+ } else if (llvm::omp::allSimdSet.test(ompDirective)) {
+ // 2.9.3.1 SIMD construct
+ createSimdLoop(converter, semaCtx, eval, ompDirective, loopOpClauseList,
+ currentLocation);
+ genOpenMPReduction(converter, semaCtx, loopOpClauseList);
+ } else {
+ createWsloop(converter, semaCtx, eval, ompDirective, loopOpClauseList,
+ endClauseList, currentLocation);
+ }
+}
+
+static void
+genOMP(Fortran::lower::AbstractConverter &converter,
+ Fortran::lower::SymMap &symTable,
+ Fortran::semantics::SemanticsContext &semaCtx,
+ Fortran::lower::pft::Evaluation &eval,
+ const Fortran::parser::OpenMPSectionConstruct §ionConstruct) {
+ // SECTION constructs are handled as a part of SECTIONS.
+ llvm_unreachable("Unexpected standalone OMP SECTION");
+}
+
static void
genOMP(Fortran::lower::AbstractConverter &converter,
Fortran::lower::SymMap &symTable,
@@ -1999,98 +2161,27 @@ genOMP(Fortran::lower::AbstractConverter &converter,
Fortran::lower::SymMap &symTable,
Fortran::semantics::SemanticsContext &semaCtx,
Fortran::lower::pft::Evaluation &eval,
- const Fortran::parser::OpenMPAtomicConstruct &atomicConstruct) {
+ const Fortran::parser::OpenMPStandaloneConstruct &standaloneConstruct) {
std::visit(
Fortran::common::visitors{
- [&](const Fortran::parser::OmpAtomicRead &atomicRead) {
- mlir::Location loc = converter.genLocation(atomicRead.source);
- Fortran::lower::genOmpAccAtomicRead<
- Fortran::parser::OmpAtomicRead,
- Fortran::parser::OmpAtomicClauseList>(converter, atomicRead,
- loc);
- },
- [&](const Fortran::parser::OmpAtomicWrite &atomicWrite) {
- mlir::Location loc = converter.genLocation(atomicWrite.source);
- Fortran::lower::genOmpAccAtomicWrite<
- Fortran::parser::OmpAtomicWrite,
- Fortran::parser::OmpAtomicClauseList>(converter, atomicWrite,
- loc);
+ [&](const Fortran::parser::OpenMPSimpleStandaloneConstruct
+ &simpleStandaloneConstruct) {
+ genOmpSimpleStandalone(converter, semaCtx, eval,
+ /*genNested=*/true,
+ simpleStandaloneConstruct);
},
- [&](const Fortran::parser::OmpAtomic &atomicConstruct) {
- mlir::Location loc = converter.genLocation(atomicConstruct.source);
- Fortran::lower::genOmpAtomic<Fortran::parser::OmpAtomic,
- Fortran::parser::OmpAtomicClauseList>(
- converter, atomicConstruct, loc);
+ [&](const Fortran::parser::OpenMPFlushConstruct &flushConstruct) {
+ genOmpFlush(converter, semaCtx, eval, flushConstruct);
},
- [&](const Fortran::parser::OmpAtomicUpdate &atomicUpdate) {
- mlir::Location loc = converter.genLocation(atomicUpdate.source);
- Fortran::lower::genOmpAccAtomicUpdate<
- Fortran::parser::OmpAtomicUpdate,
- Fortran::parser::OmpAtomicClauseList>(converter, atomicUpdate,
- loc);
+ [&](const Fortran::parser::OpenMPCancelConstruct &cancelConstruct) {
+ TODO(converter.getCurrentLocation(), "OpenMPCancelConstruct");
},
- [&](const Fortran::parser::OmpAtomicCapture &atomicCapture) {
- mlir::Location loc = converter.genLocation(atomicCapture.source);
- Fortran::lower::genOmpAccAtomicCapture<
- Fortran::parser::OmpAtomicCapture,
- Fortran::parser::OmpAtomicClauseList>(converter, atomicCapture,
- loc);
+ [&](co...
[truncated]
|
kiranchandramohan
approved these changes
Mar 24, 2024
skatrak
approved these changes
Mar 25, 2024
flang/lib/Lower/OpenMP/OpenMP.cpp
Outdated
| declareTargetOp.setDeclareTarget(deviceType, captureClause); | ||
| } | ||
|
|
||
| // OpenMPDeclarativeConstruct visitors -------------------------------- |
There was a problem hiding this comment.
Nit: Elsewhere in this file the format for comments defining different groups of functions is as follows:
//===----------------------------------------------------------------------===//
// File section name
//===----------------------------------------------------------------------===//I think it's worth modifying this and the "OpenMPConstruct visitors" instance below to use the same formatting.
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
✅ With the latest revision this PR passed the Python code formatter. |
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.
Put all of the genOMP functions together, organize them in two groups: for declarative constructs and for other (executable) constructs.
Replace visit functions for OpenMPDeclarativeConstruct and OpenMPConstruct from listing individual visitors for each variant alternative to using a single generic visitor. Essentially, going from
to
This doesn't change any functionality, just reorganizes the functions a bit. The intent here is to improve the readability of this file.