[flang][OpenMP] Move clause/object conversion to happen early, in genOMP #87086
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
This patch introduces a set of composable structures grouping the MLIR operands associated to each OpenMP clause. This makes it easier to keep the MLIR representation for the same clause consistent throughout all operations that accept it. The relevant clause operand structures are grouped into per-operation structures using a mixin pattern and used to define new operation constructors. These constructors can be used to avoid having to get the order of a possibly large list of operands right. Missing clauses are documented as TODOs, as well as operands which are part of the relevant operation's operand structure but cannot be attached to the associated operation yet, due to missing op arguments to its MLIR definition. A follow-up patch will update Flang lowering to make use of these structures, simplifying the passing of information from clause processing to operation- generating functions and also simplifying the creation of operations through the use of the new operation constructors.
This patch updates Flang lowering to use the new set of OpenMP clause operand structures and their groupings into directive-specific sets of clause operands. It simplifies the passing of information from the clause processor and the creation of operations. The `DataSharingProcessor` is slightly modified to not hold delayed privatization state. Instead, optional arguments are added to `processStep1` which are only passed when delayed privatization is used. This enables using the clause operand structure for `private` and removes the need for the ad-hoc `DelayedPrivatizationInfo` structure. The processing of the `schedule` clause is updated to process the `chunk` modifier rather than requiring two separate calls to the `ClauseProcessor`. Lowering of a block-associated `ordered` construct is updated to emit a TODO error if the `simd` clause is specified, since it is not currently supported by the `ClauseProcessor` or later compilation stages. Removed processing of `schedule` from `omp.simdloop`, as it doesn't apply to `simd` constructs.
This patch performs several cleanups with the main purpose of normalizing the code patterns used to trigger codegen for MLIR OpenMP operations and making the processing of clauses and constructs independent. The following changes are made: - Clean up unused `directive` argument to `ClauseProcessor::processMap()`. - Move general helper functions in OpenMP.cpp to the appropriate section of the file. - Create `gen<OpName>Clauses()` functions containing the clause processing code specific for the associated OpenMP construct. - Update `gen<OpName>Op()` functions to call the corresponding `gen<OpName>Clauses()` function. - Sort calls to `ClauseProcessor::process<ClauseName>()` alphabetically, to avoid inadvertently relying on some arbitrary order. Update some tests that broke due to the order change. - Normalize `genOMP()` functions so they all delegate the generation of MLIR to `gen<OpName>Op()` functions following the same pattern. - Only process `nowait` clause on `TARGET` constructs if not compiling for the target device. A later patch can move the calls to `gen<OpName>Clauses()` out of `gen<OpName>Op()` functions and passing completed clause structures instead, in preparation to supporting composite constructs. That will make it possible to reuse clause processing for a given leaf construct when appearing alone or in a combined or composite construct, while controlling where the associated code is produced.
…katrak/spr/clause-operands-02-flang
…skatrak/spr/clause-operands-03-genopclauses
This patch simplifies the lowering from PFT to MLIR of OpenMP compound constructs (i.e. combined and composite). The new approach consists of iteratively processing the outermost leaf construct of the given combined construct until it cannot be split further. Both leaf constructs and composite ones have `gen...()` functions that are called when appropriate. This approach enables treating a leaf construct the same way regardless of if it appeared as part of a combined construct, and it also enables the lowering of composite constructs as a single unit. Previous corner cases are now handled in a more straightforward way and comments pointing to the relevant spec section are added. Directive sets are also completed with missing LOOP related constructs.
llvmbot
added
flang
|
@llvm/pr-subscribers-flang-fir-hlfir Author: Krzysztof Parzyszek (kparzysz) ChangesThis removes the last use of genOmpObectList2, which has now been removed. Patch is 58.36 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/87086.diff 5 Files Affected:
diff --git a/flang/lib/Lower/OpenMP/ClauseProcessor.h b/flang/lib/Lower/OpenMP/ClauseProcessor.h
index db7a1b8335f818..f4d659b70cfee7 100644
--- a/flang/lib/Lower/OpenMP/ClauseProcessor.h
+++ b/flang/lib/Lower/OpenMP/ClauseProcessor.h
@@ -49,9 +49,8 @@ class ClauseProcessor {
public:
ClauseProcessor(Fortran::lower::AbstractConverter &converter,
Fortran::semantics::SemanticsContext &semaCtx,
- const Fortran::parser::OmpClauseList &clauses)
- : converter(converter), semaCtx(semaCtx),
- clauses(makeClauses(clauses, semaCtx)) {}
+ const List<Clause> &clauses)
+ : converter(converter), semaCtx(semaCtx), clauses(clauses) {}
// 'Unique' clauses: They can appear at most once in the clause list.
bool processCollapse(
diff --git a/flang/lib/Lower/OpenMP/DataSharingProcessor.h b/flang/lib/Lower/OpenMP/DataSharingProcessor.h
index c11ee299c5d085..ef7b14327278e3 100644
--- a/flang/lib/Lower/OpenMP/DataSharingProcessor.h
+++ b/flang/lib/Lower/OpenMP/DataSharingProcessor.h
@@ -78,13 +78,12 @@ class DataSharingProcessor {
public:
DataSharingProcessor(Fortran::lower::AbstractConverter &converter,
Fortran::semantics::SemanticsContext &semaCtx,
- const Fortran::parser::OmpClauseList &opClauseList,
+ const List<Clause> &clauses,
Fortran::lower::pft::Evaluation &eval,
bool useDelayedPrivatization = false,
Fortran::lower::SymMap *symTable = nullptr)
: hasLastPrivateOp(false), converter(converter),
- firOpBuilder(converter.getFirOpBuilder()),
- clauses(omp::makeClauses(opClauseList, semaCtx)), eval(eval),
+ firOpBuilder(converter.getFirOpBuilder()), clauses(clauses), eval(eval),
useDelayedPrivatization(useDelayedPrivatization), symTable(symTable) {}
// Privatisation is split into two steps.
diff --git a/flang/lib/Lower/OpenMP/OpenMP.cpp b/flang/lib/Lower/OpenMP/OpenMP.cpp
index edae453972d3d9..23dc25ac1ae9a1 100644
--- a/flang/lib/Lower/OpenMP/OpenMP.cpp
+++ b/flang/lib/Lower/OpenMP/OpenMP.cpp
@@ -17,6 +17,7 @@
#include "DataSharingProcessor.h"
#include "DirectivesCommon.h"
#include "ReductionProcessor.h"
+#include "Utils.h"
#include "flang/Common/idioms.h"
#include "flang/Lower/Bridge.h"
#include "flang/Lower/ConvertExpr.h"
@@ -310,14 +311,15 @@ static void getDeclareTargetInfo(
} else if (const auto *clauseList{
Fortran::parser::Unwrap<Fortran::parser::OmpClauseList>(
spec.u)}) {
- if (clauseList->v.empty()) {
+ List<Clause> clauses = makeClauses(*clauseList, semaCtx);
+ if (clauses.empty()) {
// Case: declare target, implicit capture of function
symbolAndClause.emplace_back(
mlir::omp::DeclareTargetCaptureClause::to,
eval.getOwningProcedure()->getSubprogramSymbol());
}
- ClauseProcessor cp(converter, semaCtx, *clauseList);
+ ClauseProcessor cp(converter, semaCtx, clauses);
cp.processDeviceType(clauseOps);
cp.processEnter(symbolAndClause);
cp.processLink(symbolAndClause);
@@ -597,14 +599,11 @@ static void removeStoreOp(mlir::Operation *reductionOp, mlir::Value symVal) {
// TODO: Generate the reduction operation during lowering instead of creating
// and removing operations since this is not a robust approach. Also, removing
// ops in the builder (instead of a rewriter) is probably not the best approach.
-static void
-genOpenMPReduction(Fortran::lower::AbstractConverter &converter,
- Fortran::semantics::SemanticsContext &semaCtx,
- const Fortran::parser::OmpClauseList &clauseList) {
+static void genOpenMPReduction(Fortran::lower::AbstractConverter &converter,
+ Fortran::semantics::SemanticsContext &semaCtx,
+ const List<Clause> &clauses) {
fir::FirOpBuilder &firOpBuilder = converter.getFirOpBuilder();
- List<Clause> clauses{makeClauses(clauseList, semaCtx)};
-
for (const Clause &clause : clauses) {
if (const auto &reductionClause =
std::get_if<clause::Reduction>(&clause.u)) {
@@ -812,7 +811,7 @@ struct OpWithBodyGenInfo {
return *this;
}
- OpWithBodyGenInfo &setClauses(const Fortran::parser::OmpClauseList *value) {
+ OpWithBodyGenInfo &setClauses(const List<Clause> *value) {
clauses = value;
return *this;
}
@@ -848,7 +847,7 @@ struct OpWithBodyGenInfo {
/// [in] is this an outer operation - prevents privatization.
bool outerCombined = false;
/// [in] list of clauses to process.
- const Fortran::parser::OmpClauseList *clauses = nullptr;
+ const List<Clause> *clauses = nullptr;
/// [in] if provided, processes the construct's data-sharing attributes.
DataSharingProcessor *dsp = nullptr;
/// [in] if provided, list of reduction symbols
@@ -1226,36 +1225,33 @@ static OpTy genOpWithBody(OpWithBodyGenInfo &info, Args &&...args) {
// Code generation functions for clauses
//===----------------------------------------------------------------------===//
-static void genCriticalDeclareClauses(
- Fortran::lower::AbstractConverter &converter,
- Fortran::semantics::SemanticsContext &semaCtx,
- const Fortran::parser::OmpClauseList &clauses, mlir::Location loc,
- mlir::omp::CriticalClauseOps &clauseOps, llvm::StringRef name) {
+static void
+genCriticalDeclareClauses(Fortran::lower::AbstractConverter &converter,
+ Fortran::semantics::SemanticsContext &semaCtx,
+ const List<Clause> &clauses, mlir::Location loc,
+ mlir::omp::CriticalClauseOps &clauseOps,
+ llvm::StringRef name) {
ClauseProcessor cp(converter, semaCtx, clauses);
cp.processHint(clauseOps);
clauseOps.nameAttr =
mlir::StringAttr::get(converter.getFirOpBuilder().getContext(), name);
}
-static void genFlushClauses(
- Fortran::lower::AbstractConverter &converter,
- Fortran::semantics::SemanticsContext &semaCtx,
- const std::optional<Fortran::parser::OmpObjectList> &objects,
- const std::optional<std::list<Fortran::parser::OmpMemoryOrderClause>>
- &clauses,
- mlir::Location loc, llvm::SmallVectorImpl<mlir::Value> &operandRange) {
- if (objects)
- genObjectList2(*objects, converter, operandRange);
-
- if (clauses && clauses->size() > 0)
+static void genFlushClauses(Fortran::lower::AbstractConverter &converter,
+ Fortran::semantics::SemanticsContext &semaCtx,
+ const ObjectList &objects,
+ const List<Clause> &clauses, mlir::Location loc,
+ llvm::SmallVectorImpl<mlir::Value> &operandRange) {
+ genObjectList(objects, converter, operandRange);
+
+ if (clauses.size() > 0)
TODO(converter.getCurrentLocation(), "Handle OmpMemoryOrderClause");
}
static void
genOrderedRegionClauses(Fortran::lower::AbstractConverter &converter,
Fortran::semantics::SemanticsContext &semaCtx,
- const Fortran::parser::OmpClauseList &clauses,
- mlir::Location loc,
+ const List<Clause> &clauses, mlir::Location loc,
mlir::omp::OrderedRegionClauseOps &clauseOps) {
ClauseProcessor cp(converter, semaCtx, clauses);
cp.processTODO<clause::Simd>(loc, llvm::omp::Directive::OMPD_ordered);
@@ -1264,9 +1260,9 @@ genOrderedRegionClauses(Fortran::lower::AbstractConverter &converter,
static void genParallelClauses(
Fortran::lower::AbstractConverter &converter,
Fortran::semantics::SemanticsContext &semaCtx,
- Fortran::lower::StatementContext &stmtCtx,
- const Fortran::parser::OmpClauseList &clauses, mlir::Location loc,
- bool processReduction, mlir::omp::ParallelClauseOps &clauseOps,
+ Fortran::lower::StatementContext &stmtCtx, const List<Clause> &clauses,
+ mlir::Location loc, bool processReduction,
+ mlir::omp::ParallelClauseOps &clauseOps,
llvm::SmallVectorImpl<mlir::Type> &reductionTypes,
llvm::SmallVectorImpl<const Fortran::semantics::Symbol *> &reductionSyms) {
ClauseProcessor cp(converter, semaCtx, clauses);
@@ -1286,8 +1282,7 @@ static void genParallelClauses(
static void genSectionsClauses(Fortran::lower::AbstractConverter &converter,
Fortran::semantics::SemanticsContext &semaCtx,
- const Fortran::parser::OmpClauseList &clauses,
- mlir::Location loc,
+ const List<Clause> &clauses, mlir::Location loc,
bool clausesFromBeginSections,
mlir::omp::SectionsClauseOps &clauseOps) {
ClauseProcessor cp(converter, semaCtx, clauses);
@@ -1304,9 +1299,8 @@ static void genSimdLoopClauses(
Fortran::lower::AbstractConverter &converter,
Fortran::semantics::SemanticsContext &semaCtx,
Fortran::lower::StatementContext &stmtCtx,
- Fortran::lower::pft::Evaluation &eval,
- const Fortran::parser::OmpClauseList &clauses, mlir::Location loc,
- mlir::omp::SimdLoopClauseOps &clauseOps,
+ Fortran::lower::pft::Evaluation &eval, const List<Clause> &clauses,
+ mlir::Location loc, mlir::omp::SimdLoopClauseOps &clauseOps,
llvm::SmallVectorImpl<const Fortran::semantics::Symbol *> &iv) {
ClauseProcessor cp(converter, semaCtx, clauses);
cp.processCollapse(loc, eval, clauseOps, iv);
@@ -1324,9 +1318,8 @@ static void genSimdLoopClauses(
static void genSingleClauses(Fortran::lower::AbstractConverter &converter,
Fortran::semantics::SemanticsContext &semaCtx,
- const Fortran::parser::OmpClauseList &beginClauses,
- const Fortran::parser::OmpClauseList &endClauses,
- mlir::Location loc,
+ const List<Clause> &beginClauses,
+ const List<Clause> &endClauses, mlir::Location loc,
mlir::omp::SingleClauseOps &clauseOps) {
ClauseProcessor bcp(converter, semaCtx, beginClauses);
bcp.processAllocate(clauseOps);
@@ -1340,9 +1333,8 @@ static void genSingleClauses(Fortran::lower::AbstractConverter &converter,
static void genTargetClauses(
Fortran::lower::AbstractConverter &converter,
Fortran::semantics::SemanticsContext &semaCtx,
- Fortran::lower::StatementContext &stmtCtx,
- const Fortran::parser::OmpClauseList &clauses, mlir::Location loc,
- bool processHostOnlyClauses, bool processReduction,
+ Fortran::lower::StatementContext &stmtCtx, const List<Clause> &clauses,
+ mlir::Location loc, bool processHostOnlyClauses, bool processReduction,
mlir::omp::TargetClauseOps &clauseOps,
llvm::SmallVectorImpl<const Fortran::semantics::Symbol *> &mapSyms,
llvm::SmallVectorImpl<mlir::Location> &mapSymLocs,
@@ -1368,9 +1360,8 @@ static void genTargetClauses(
static void genTargetDataClauses(
Fortran::lower::AbstractConverter &converter,
Fortran::semantics::SemanticsContext &semaCtx,
- Fortran::lower::StatementContext &stmtCtx,
- const Fortran::parser::OmpClauseList &clauses, mlir::Location loc,
- mlir::omp::TargetDataClauseOps &clauseOps,
+ Fortran::lower::StatementContext &stmtCtx, const List<Clause> &clauses,
+ mlir::Location loc, mlir::omp::TargetDataClauseOps &clauseOps,
llvm::SmallVectorImpl<mlir::Type> &useDeviceTypes,
llvm::SmallVectorImpl<mlir::Location> &useDeviceLocs,
llvm::SmallVectorImpl<const Fortran::semantics::Symbol *> &useDeviceSyms) {
@@ -1401,9 +1392,8 @@ static void genTargetDataClauses(
static void genTargetEnterExitUpdateDataClauses(
Fortran::lower::AbstractConverter &converter,
Fortran::semantics::SemanticsContext &semaCtx,
- Fortran::lower::StatementContext &stmtCtx,
- const Fortran::parser::OmpClauseList &clauses, mlir::Location loc,
- llvm::omp::Directive directive,
+ Fortran::lower::StatementContext &stmtCtx, const List<Clause> &clauses,
+ mlir::Location loc, llvm::omp::Directive directive,
mlir::omp::TargetEnterExitUpdateDataClauseOps &clauseOps) {
ClauseProcessor cp(converter, semaCtx, clauses);
cp.processDepend(clauseOps);
@@ -1422,8 +1412,7 @@ static void genTargetEnterExitUpdateDataClauses(
static void genTaskClauses(Fortran::lower::AbstractConverter &converter,
Fortran::semantics::SemanticsContext &semaCtx,
Fortran::lower::StatementContext &stmtCtx,
- const Fortran::parser::OmpClauseList &clauses,
- mlir::Location loc,
+ const List<Clause> &clauses, mlir::Location loc,
mlir::omp::TaskClauseOps &clauseOps) {
ClauseProcessor cp(converter, semaCtx, clauses);
cp.processAllocate(clauseOps);
@@ -1442,8 +1431,7 @@ static void genTaskClauses(Fortran::lower::AbstractConverter &converter,
static void genTaskgroupClauses(Fortran::lower::AbstractConverter &converter,
Fortran::semantics::SemanticsContext &semaCtx,
- const Fortran::parser::OmpClauseList &clauses,
- mlir::Location loc,
+ const List<Clause> &clauses, mlir::Location loc,
mlir::omp::TaskgroupClauseOps &clauseOps) {
ClauseProcessor cp(converter, semaCtx, clauses);
cp.processAllocate(clauseOps);
@@ -1453,8 +1441,7 @@ static void genTaskgroupClauses(Fortran::lower::AbstractConverter &converter,
static void genTaskwaitClauses(Fortran::lower::AbstractConverter &converter,
Fortran::semantics::SemanticsContext &semaCtx,
- const Fortran::parser::OmpClauseList &clauses,
- mlir::Location loc,
+ const List<Clause> &clauses, mlir::Location loc,
mlir::omp::TaskwaitClauseOps &clauseOps) {
ClauseProcessor cp(converter, semaCtx, clauses);
cp.processTODO<clause::Depend, clause::Nowait>(
@@ -1464,8 +1451,7 @@ static void genTaskwaitClauses(Fortran::lower::AbstractConverter &converter,
static void genTeamsClauses(Fortran::lower::AbstractConverter &converter,
Fortran::semantics::SemanticsContext &semaCtx,
Fortran::lower::StatementContext &stmtCtx,
- const Fortran::parser::OmpClauseList &clauses,
- mlir::Location loc,
+ const List<Clause> &clauses, mlir::Location loc,
mlir::omp::TeamsClauseOps &clauseOps) {
ClauseProcessor cp(converter, semaCtx, clauses);
cp.processAllocate(clauseOps);
@@ -1482,9 +1468,8 @@ static void genWsloopClauses(
Fortran::lower::AbstractConverter &converter,
Fortran::semantics::SemanticsContext &semaCtx,
Fortran::lower::StatementContext &stmtCtx,
- Fortran::lower::pft::Evaluation &eval,
- const Fortran::parser::OmpClauseList &beginClauses,
- const Fortran::parser::OmpClauseList *endClauses, mlir::Location loc,
+ Fortran::lower::pft::Evaluation &eval, const List<Clause> &beginClauses,
+ const List<Clause> &endClauses, mlir::Location loc,
mlir::omp::WsloopClauseOps &clauseOps,
llvm::SmallVectorImpl<const Fortran::semantics::Symbol *> &iv,
llvm::SmallVectorImpl<mlir::Type> &reductionTypes,
@@ -1501,8 +1486,8 @@ static void genWsloopClauses(
if (ReductionProcessor::doReductionByRef(clauseOps.reductionVars))
clauseOps.reductionByRefAttr = firOpBuilder.getUnitAttr();
- if (endClauses) {
- ClauseProcessor ecp(converter, semaCtx, *endClauses);
+ if (!endClauses.empty()) {
+ ClauseProcessor ecp(converter, semaCtx, endClauses);
ecp.processNowait(clauseOps);
}
@@ -1525,8 +1510,7 @@ static mlir::omp::CriticalOp
genCriticalOp(Fortran::lower::AbstractConverter &converter,
Fortran::semantics::SemanticsContext &semaCtx,
Fortran::lower::pft::Evaluation &eval, bool genNested,
- mlir::Location loc,
- const Fortran::parser::OmpClauseList &clauseList,
+ mlir::Location loc, const List<Clause> &clauses,
const std::optional<Fortran::parser::Name> &name) {
fir::FirOpBuilder &firOpBuilder = converter.getFirOpBuilder();
mlir::FlatSymbolRefAttr nameAttr;
@@ -1537,7 +1521,7 @@ genCriticalOp(Fortran::lower::AbstractConverter &converter,
auto global = mod.lookupSymbol<mlir::omp::CriticalDeclareOp>(nameStr);
if (!global) {
mlir::omp::CriticalClauseOps clauseOps;
- genCriticalDeclareClauses(converter, semaCtx, clauseList, loc, clauseOps,
+ genCriticalDeclareClauses(converter, semaCtx, clauses, loc, clauseOps,
nameStr);
mlir::OpBuilder modBuilder(mod.getBodyRegion());
@@ -1556,8 +1540,7 @@ static mlir::omp::DistributeOp
genDistributeOp(Fortran::lower::AbstractConverter &converter,
Fortran::semantics::SemanticsContext &semaCtx,
Fortran::lower::pft::Evaluation &eval, bool genNested,
- mlir::Location loc,
- const Fortran::parser::OmpClauseList &clauseList) {
+ mlir::Location loc, const List<Clause> &clauses) {
TODO(loc, "Distribute construct");
return nullptr;
}
@@ -1566,12 +1549,9 @@ static mlir::omp::FlushOp
genFlushOp(Fortran::lower::AbstractConverter &converter,
Fortran::semantics::SemanticsContext &semaCtx,
Fortran::lower::pft::Evaluation &eval, mlir::Location loc,
- const std::optional<Fortran::parser::OmpObjectList> &objectList,
- const std::optional<std::list<Fortran::parser::OmpMemoryOrderClause>>
- &clauseList) {
+ const ObjectList &objects, const List<Clause> &clauses) {
llvm::SmallVector<mlir::Value> operandRange;
- genFlushClauses(converter, semaCtx, objectList, clauseList, loc,
- operandRange);
+ genFlushClauses(converter, semaCtx, objects, clauses, loc, operandRange);
return converter.getFirOpBuilder().create<mlir::omp::FlushOp>(
converter.getCurrentLocation(), operandRange);
@@ -1591,7 +1571,7 @@ static mlir::omp::OrderedOp
genOrderedOp(Fortran::lower::AbstractConverter &converter,
Fortran::semantics::SemanticsContext &semaCtx,
Fortran::lower::pft::Evaluation &eval, mlir::Location loc,
- const Fortran::parser::OmpClauseList &clauseList) {
+ const List<Clause> &clauses) {
TODO(loc, "OMPD_ordered");
return nullptr;
}
@@ -1600,10 +1580,9 @@ static mlir::omp::OrderedRegionOp
genOrderedRegionOp(Fortran::lower::AbstractConverter &converter,
Fortran::semantics::SemanticsContext &semaCtx,
Fortran::lower::pft::Evaluation &eval, bool genNested,
- mlir::Location loc,
- const Fortran::parser::OmpClauseList &clauseList) {
+ mlir::Location loc, const List<Clause> &clauses) {
mlir::omp::OrderedRegionClauseOps clauseOps;
- genOrderedRegionClauses(converter, semaCtx, clauseList, loc, clauseOps);
+ genOrderedRegionClauses(converter, semaCtx, clauses, loc, clauseOps);
return genOpWithBody<mlir::omp::OrderedRegionOp>(
OpWithBodyGenInfo(converter, semaCtx, loc, eval).setGenNested(genNested),
@@ -1615,8 +1594,7 @@ genParallelOp(Fortran::lower::AbstractConverter &converter,
Fortran::lower::SymMap &symTable,
Fortran::semantics::SemanticsContext &semaCtx,
Fortran::lower::pft::Evaluation &eval, bool genNested,
- mlir::Location loc,
- const Fortran::parser::OmpClauseList &clauseList,
+ mlir::Location loc, const List<Clause> &clauses,
bool outerCombined = false) {
fir::FirOpBuilder &firOpBuilder = converter.getFirOpBuilder();
Fortran::lower::StatementContext stmtCtx;
@@ -1624,7 +1602,7 @@ genParallelOp(Fortran::lower::AbstractConverter &converter,
llvm::SmallVector<const Fortran::semantics::Symbol *> privateSyms;
llvm::SmallVector<mlir::Type> red...
[truncated]
|
skatrak
approved these changes
Apr 2, 2024
| @@ -58,17 +58,15 @@ void gatherFuncAndVarSyms( | |||
| const ObjectList &objects, mlir::omp::DeclareTargetCaptureClause clause, | |||
| llvm::SmallVectorImpl<DeclareTargetCapturePair> &symbolAndClause); | |||
|
|
|||
| int64_t getCollapseValue(const List<Clause> &clauses); | |||
There was a problem hiding this comment.
It looks like this function is only used within OpenMP.cpp, is it expected to be used elsewhere later? Otherwise it may make more sense to make it internal (static) to that compilation unit instead.
| : converter(converter), semaCtx(semaCtx), | ||
| clauses(makeClauses(clauses, semaCtx)) {} | ||
| const List<Clause> &clauses) | ||
| : converter(converter), semaCtx(semaCtx), clauses(clauses) {} |
There was a problem hiding this comment.
Could we make the clauses field an ArrayRef<Clause>? In the regular use pattern of this class the clause list of the caller does not go out of scope before the ClauseProcessor instance does. Maybe we can document this and avoid creating a local copy.
In any case this is a discussion to consider creating another patch for, not something to be addressed here.
flang/lib/Lower/OpenMP/OpenMP.cpp
Outdated
| const Fortran::parser::OmpClauseList &clauseList) { | ||
| static void genOpenMPReduction(Fortran::lower::AbstractConverter &converter, | ||
| Fortran::semantics::SemanticsContext &semaCtx, | ||
| const List<Clause> &clauses) { |
There was a problem hiding this comment.
Nit: I'm generally in favor of using ArrayRef<T> in place of const Container<T> & for function arguments in all gen... functions here and the constructor for ClauseProcessor, but if you don't agree I don't think it's a reason to withhold approval either.
| !std::get_if<Fortran::parser::OmpClause::ThreadLimit>(&clause.u) && | ||
| !std::get_if<Fortran::parser::OmpClause::NumTeams>(&clause.u) && | ||
| !std::get_if<Fortran::parser::OmpClause::Simd>(&clause.u)) { | ||
| if (!std::get_if<clause::If>(&clause.u) && |
There was a problem hiding this comment.
Nit: I'd suggest taking the opportunity of already updating all these lines to sort them alphabetically and possibly replace std::get_if with std::holds_alternative, but feel free to ignore (same thing below for endClauses).
There was a problem hiding this comment.
I did that in #89090.
|
We can switch to ArrayRef in another PR. |
Base automatically changed from
users/skatrak/spr/clause-operands-04-compound-lower
to
main
April 17, 2024 11:17
This removes the last use of genOmpObjectList2, which has now been removed.
kparzysz
force-pushed
the
users/kparzysz/spr/a05-makeclause
branch
from
f725fac to
de93771
Compare
|
✅ With the latest revision this PR passed the C/C++ 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.
This removes the last use of genOmpObjectList2, which has now been removed.