Skip to content

[flang][OpenMP][NFC] Outline genOpWithBody & createBodyOfOp args #80817

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 6, 2024
Merged

[flang][OpenMP][NFC] Outline genOpWithBody & createBodyOfOp args #80817

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
162 changes: 86 additions & 76 deletions flang/lib/Lower/OpenMP.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2250,6 +2250,17 @@ createAndSetPrivatizedLoopVar(Fortran::lower::AbstractConverter &converter,
return storeOp;
}

struct CreateBodyOfOpInfo {
Fortran::lower::AbstractConverter &converter;
mlir::Location &loc;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
mlir::Location &loc;
mlir::Location loc;

Fortran::lower::pft::Evaluation &eval;
bool genNested = true;
const Fortran::parser::OmpClauseList *clauses = nullptr;
const llvm::SmallVector<const Fortran::semantics::Symbol *> &args = {};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const llvm::SmallVector<const Fortran::semantics::Symbol *> &args = {};
const llvm::ArrayRef<const Fortran::semantics::Symbol *> args = {};

bool outerCombined = false;
DataSharingProcessor *dsp = nullptr;
Comment on lines +2257 to +2261
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we can't use member designators, what about creating setters for the fields that have a default value?

  CreateBodyOfOpInfo &setGenNested(bool val) { genNested = val; }
  CreateBodyOfOpInfo &setClauses(const Fortran::parser::OmpClauseList *clauses) { clauses = clauses; }
  ...

By returning a reference to the object, it should be possible to chain calls to setters, like below:
{ converter, loc, eval }.setClauses(clauses).setDsp(dsp)

The advantage of this is that only fields with non-default values need to be explicitly set.
So, for instance, if a given function just needs to set dsp, it doesn't need to specify all other fields before it.
This makes it easier to add a new field or change default values.

Another option would be to add constructors that handle the most commonly set fields.
Example:

CreateBodyOfOpInfo(Fortran::lower::AbstractConverter &converter,
  mlir::Location &loc,
  Fortran::lower::pft::Evaluation &eval,
  bool genNested = true,
  const Fortran::parser::OmpClauseList *clauses = nullptr,
  const llvm::SmallVector<const Fortran::semantics::Symbol *> &args = {},
  bool outerCombined = false,
  DataSharingProcessor *dsp = nullptr)
  : loc(loc), eval(eval), genNested(genNested), ... {}

CreateBodyOfOpInfo(Fortran::lower::AbstractConverter &converter,
  mlir::Location &loc,
  Fortran::lower::pft::Evaluation &eval,
  bool genNested,
  bool outerCombined)
  : CreateBodyOfOpInfo(converter, loc, eval, genNested, nullptr, {}, outerCombined) {}

CreateBodyOfOpInfo(Fortran::lower::AbstractConverter &converter,
  mlir::Location &loc,
  Fortran::lower::pft::Evaluation &eval,
  DataSharingProcessor *dsp)
  : CreateBodyOfOpInfo(converter, loc, eval, true, nullptr, {}, false, dsp) {}

};

/// Create the body (block) for an OpenMP Operation.
///
/// \param [in] op - the operation the body belongs to.
Expand All @@ -2263,13 +2274,8 @@ createAndSetPrivatizedLoopVar(Fortran::lower::AbstractConverter &converter,
/// \param [in] outerCombined - is this an outer operation - prevents
/// privatization.
template <typename Op>
static void createBodyOfOp(
Op &op, Fortran::lower::AbstractConverter &converter, mlir::Location &loc,
Fortran::lower::pft::Evaluation &eval, bool genNested,
const Fortran::parser::OmpClauseList *clauses = nullptr,
const llvm::SmallVector<const Fortran::semantics::Symbol *> &args = {},
bool outerCombined = false, DataSharingProcessor *dsp = nullptr) {
fir::FirOpBuilder &firOpBuilder = converter.getFirOpBuilder();
static void createBodyOfOp(Op &op, CreateBodyOfOpInfo info) {
fir::FirOpBuilder &firOpBuilder = info.converter.getFirOpBuilder();

auto insertMarker = [](fir::FirOpBuilder &builder) {
mlir::Value undef = builder.create<fir::UndefOp>(builder.getUnknownLoc(),
Expand All @@ -2281,22 +2287,22 @@ static void createBodyOfOp(
// argument. Also update the symbol's address with the mlir argument value.
// e.g. For loops the argument is the induction variable. And all further
// uses of the induction variable should use this mlir value.
if (args.size()) {
if (info.args.size()) {
std::size_t loopVarTypeSize = 0;
for (const Fortran::semantics::Symbol *arg : args)
for (const Fortran::semantics::Symbol *arg : info.args)
loopVarTypeSize = std::max(loopVarTypeSize, arg->GetUltimate().size());
mlir::Type loopVarType = getLoopVarType(converter, loopVarTypeSize);
llvm::SmallVector<mlir::Type> tiv(args.size(), loopVarType);
llvm::SmallVector<mlir::Location> locs(args.size(), loc);
mlir::Type loopVarType = getLoopVarType(info.converter, loopVarTypeSize);
llvm::SmallVector<mlir::Type> tiv(info.args.size(), loopVarType);
llvm::SmallVector<mlir::Location> locs(info.args.size(), info.loc);
firOpBuilder.createBlock(&op.getRegion(), {}, tiv, locs);
// The argument is not currently in memory, so make a temporary for the
// argument, and store it there, then bind that location to the argument.
mlir::Operation *storeOp = nullptr;
for (auto [argIndex, argSymbol] : llvm::enumerate(args)) {
for (auto [argIndex, argSymbol] : llvm::enumerate(info.args)) {
mlir::Value indexVal =
fir::getBase(op.getRegion().front().getArgument(argIndex));
storeOp =
createAndSetPrivatizedLoopVar(converter, loc, indexVal, argSymbol);
storeOp = createAndSetPrivatizedLoopVar(info.converter, info.loc,
indexVal, argSymbol);
}
firOpBuilder.setInsertionPointAfter(storeOp);
} else {
Expand All @@ -2308,44 +2314,44 @@ static void createBodyOfOp(

// If it is an unstructured region and is not the outer region of a combined
// construct, create empty blocks for all evaluations.
if (eval.lowerAsUnstructured() && !outerCombined)
if (info.eval.lowerAsUnstructured() && !info.outerCombined)
Fortran::lower::createEmptyRegionBlocks<mlir::omp::TerminatorOp,
mlir::omp::YieldOp>(
firOpBuilder, eval.getNestedEvaluations());
firOpBuilder, info.eval.getNestedEvaluations());

// Start with privatization, so that the lowering of the nested
// code will use the right symbols.
constexpr bool isLoop = std::is_same_v<Op, mlir::omp::WsLoopOp> ||
std::is_same_v<Op, mlir::omp::SimdLoopOp>;
bool privatize = clauses && !outerCombined;
bool privatize = info.clauses && !info.outerCombined;

firOpBuilder.setInsertionPoint(marker);
std::optional<DataSharingProcessor> tempDsp;
if (privatize) {
if (!dsp) {
tempDsp.emplace(converter, *clauses, eval);
if (!info.dsp) {
tempDsp.emplace(info.converter, *info.clauses, info.eval);
tempDsp->processStep1();
}
}

if constexpr (std::is_same_v<Op, mlir::omp::ParallelOp>) {
threadPrivatizeVars(converter, eval);
if (clauses) {
threadPrivatizeVars(info.converter, info.eval);
if (info.clauses) {
firOpBuilder.setInsertionPoint(marker);
ClauseProcessor(converter, *clauses).processCopyin();
ClauseProcessor(info.converter, *info.clauses).processCopyin();
}
}

if (genNested) {
if (info.genNested) {
// genFIR(Evaluation&) tries to patch up unterminated blocks, causing
// a lot of complications for our approach if the terminator generation
// is delayed past this point. Insert a temporary terminator here, then
// delete it.
firOpBuilder.setInsertionPointToEnd(&op.getRegion().back());
auto *temp = Fortran::lower::genOpenMPTerminator(firOpBuilder,
op.getOperation(), loc);
auto *temp = Fortran::lower::genOpenMPTerminator(
firOpBuilder, op.getOperation(), info.loc);
firOpBuilder.setInsertionPointAfter(marker);
genNestedEvaluations(converter, eval);
genNestedEvaluations(info.converter, info.eval);
temp->erase();
}

Expand Down Expand Up @@ -2380,28 +2386,28 @@ static void createBodyOfOp(
mlir::Block *exit = firOpBuilder.createBlock(&region);
for (mlir::Block *b : exits) {
firOpBuilder.setInsertionPointToEnd(b);
firOpBuilder.create<mlir::cf::BranchOp>(loc, exit);
firOpBuilder.create<mlir::cf::BranchOp>(info.loc, exit);
}
return exit;
};

if (auto *exitBlock = getUniqueExit(op.getRegion())) {
firOpBuilder.setInsertionPointToEnd(exitBlock);
auto *term = Fortran::lower::genOpenMPTerminator(firOpBuilder,
op.getOperation(), loc);
auto *term = Fortran::lower::genOpenMPTerminator(
firOpBuilder, op.getOperation(), info.loc);
// Only insert lastprivate code when there actually is an exit block.
// Such a block may not exist if the nested code produced an infinite
// loop (this may not make sense in production code, but a user could
// write that and we should handle it).
firOpBuilder.setInsertionPoint(term);
if (privatize) {
if (!dsp) {
if (!info.dsp) {
assert(tempDsp.has_value());
tempDsp->processStep2(op, isLoop);
} else {
if (isLoop && args.size() > 0)
dsp->setLoopIV(converter.getSymbolAddress(*args[0]));
dsp->processStep2(op, isLoop);
if (isLoop && info.args.size() > 0)
info.dsp->setLoopIV(info.converter.getSymbolAddress(*info.args[0]));
info.dsp->processStep2(op, isLoop);
}
}
}
Expand Down Expand Up @@ -2475,39 +2481,43 @@ static void genBodyOfTargetDataOp(
genNestedEvaluations(converter, eval);
}

struct GenOpWithBodyInfo {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This structure contains the same fields as CreateBodyOfOpInfo except for the args and dsp fields. Maybe it makes sense to have that structure inherit from this one so we're not using different names/types for the same things (e.g. Location &loc vs Location currentLocation).

Fortran::lower::AbstractConverter &converter;
Fortran::lower::pft::Evaluation &eval;
bool genNested = false;
mlir::Location currentLocation;
bool outerCombined = false;
const Fortran::parser::OmpClauseList *clauseList = nullptr;
};

template <typename OpTy, typename... Args>
static OpTy genOpWithBody(Fortran::lower::AbstractConverter &converter,
Fortran::lower::pft::Evaluation &eval, bool genNested,
mlir::Location currentLocation, bool outerCombined,
const Fortran::parser::OmpClauseList *clauseList,
Args &&...args) {
auto op = converter.getFirOpBuilder().create<OpTy>(
currentLocation, std::forward<Args>(args)...);
createBodyOfOp<OpTy>(op, converter, currentLocation, eval, genNested,
clauseList,
/*args=*/{}, outerCombined);
static OpTy genOpWithBody(GenOpWithBodyInfo info, Args &&...args) {
auto op = info.converter.getFirOpBuilder().create<OpTy>(
info.currentLocation, std::forward<Args>(args)...);
createBodyOfOp<OpTy>(
op, {info.converter, info.currentLocation, info.eval, info.genNested,
info.clauseList,
/*args*/ llvm::SmallVector<const Fortran::semantics::Symbol *>{},
info.outerCombined});
return op;
}

static mlir::omp::MasterOp
genMasterOp(Fortran::lower::AbstractConverter &converter,
Fortran::lower::pft::Evaluation &eval, bool genNested,
mlir::Location currentLocation) {
return genOpWithBody<mlir::omp::MasterOp>(converter, eval, genNested,
currentLocation,
/*outerCombined=*/false,
/*clauseList=*/nullptr,
/*resultTypes=*/mlir::TypeRange());
return genOpWithBody<mlir::omp::MasterOp>(
{converter, eval, genNested, currentLocation},
/*resultTypes=*/mlir::TypeRange());
}

static mlir::omp::OrderedRegionOp
genOrderedRegionOp(Fortran::lower::AbstractConverter &converter,
Fortran::lower::pft::Evaluation &eval, bool genNested,
mlir::Location currentLocation) {
return genOpWithBody<mlir::omp::OrderedRegionOp>(
converter, eval, genNested, currentLocation,
/*outerCombined=*/false,
/*clauseList=*/nullptr, /*simd=*/false);
{converter, eval, genNested, currentLocation},
/*simd=*/false);
}

static mlir::omp::ParallelOp
Expand All @@ -2534,7 +2544,7 @@ genParallelOp(Fortran::lower::AbstractConverter &converter,
cp.processReduction(currentLocation, reductionVars, reductionDeclSymbols);

return genOpWithBody<mlir::omp::ParallelOp>(
converter, eval, genNested, currentLocation, outerCombined, &clauseList,
{converter, eval, genNested, currentLocation, outerCombined, &clauseList},
/*resultTypes=*/mlir::TypeRange(), ifClauseOperand,
numThreadsClauseOperand, allocateOperands, allocatorOperands,
reductionVars,
Expand All @@ -2553,8 +2563,8 @@ genSectionOp(Fortran::lower::AbstractConverter &converter,
// Currently only private/firstprivate clause is handled, and
// all privatization is done within `omp.section` operations.
return genOpWithBody<mlir::omp::SectionOp>(
converter, eval, genNested, currentLocation,
/*outerCombined=*/false, &sectionsClauseList);
{converter, eval, genNested, currentLocation,
/*outerCombined=*/false, &sectionsClauseList});
}

static mlir::omp::SingleOp
Expand All @@ -2574,9 +2584,9 @@ genSingleOp(Fortran::lower::AbstractConverter &converter,
ClauseProcessor(converter, endClauseList).processNowait(nowaitAttr);

return genOpWithBody<mlir::omp::SingleOp>(
converter, eval, genNested, currentLocation,
/*outerCombined=*/false, &beginClauseList, allocateOperands,
allocatorOperands, nowaitAttr);
{converter, eval, genNested, currentLocation,
/*outerCombined=*/false, &beginClauseList},
allocateOperands, allocatorOperands, nowaitAttr);
}

static mlir::omp::TaskOp
Expand Down Expand Up @@ -2607,9 +2617,9 @@ genTaskOp(Fortran::lower::AbstractConverter &converter,
currentLocation, llvm::omp::Directive::OMPD_task);

return genOpWithBody<mlir::omp::TaskOp>(
converter, eval, genNested, currentLocation,
/*outerCombined=*/false, &clauseList, ifClauseOperand, finalClauseOperand,
untiedAttr, mergeableAttr,
{converter, eval, genNested, currentLocation,
/*outerCombined=*/false, &clauseList},
ifClauseOperand, finalClauseOperand, untiedAttr, mergeableAttr,
/*in_reduction_vars=*/mlir::ValueRange(),
/*in_reductions=*/nullptr, priorityClauseOperand,
dependTypeOperands.empty()
Expand All @@ -2630,8 +2640,8 @@ genTaskGroupOp(Fortran::lower::AbstractConverter &converter,
cp.processTODO<Fortran::parser::OmpClause::TaskReduction>(
currentLocation, llvm::omp::Directive::OMPD_taskgroup);
return genOpWithBody<mlir::omp::TaskGroupOp>(
converter, eval, genNested, currentLocation,
/*outerCombined=*/false, &clauseList,
{converter, eval, genNested, currentLocation,
/*outerCombined=*/false, &clauseList},
/*task_reduction_vars=*/mlir::ValueRange(),
/*task_reductions=*/nullptr, allocateOperands, allocatorOperands);
}
Expand Down Expand Up @@ -3014,7 +3024,7 @@ genTeamsOp(Fortran::lower::AbstractConverter &converter,
currentLocation, llvm::omp::Directive::OMPD_teams);

return genOpWithBody<mlir::omp::TeamsOp>(
converter, eval, genNested, currentLocation, outerCombined, &clauseList,
{converter, eval, genNested, currentLocation, outerCombined, &clauseList},
/*num_teams_lower=*/nullptr, numTeamsClauseOperand, ifClauseOperand,
threadLimitClauseOperand, allocateOperands, allocatorOperands,
reductionVars,
Expand Down Expand Up @@ -3258,9 +3268,10 @@ createSimdLoop(Fortran::lower::AbstractConverter &converter,

auto *nestedEval = getCollapsedLoopEval(
eval, Fortran::lower::getCollapseValue(loopOpClauseList));
createBodyOfOp<mlir::omp::SimdLoopOp>(simdLoopOp, converter, loc, *nestedEval,
/*genNested=*/true, &loopOpClauseList,
iv, /*outer=*/false, &dsp);
createBodyOfOp<mlir::omp::SimdLoopOp>(
simdLoopOp, {converter, loc, *nestedEval,
/*genNested=*/true, &loopOpClauseList, iv,
/*outerCombined=*/false, &dsp});
}

static void createWsLoop(Fortran::lower::AbstractConverter &converter,
Expand Down Expand Up @@ -3333,9 +3344,10 @@ static void createWsLoop(Fortran::lower::AbstractConverter &converter,

auto *nestedEval = getCollapsedLoopEval(
eval, Fortran::lower::getCollapseValue(beginClauseList));
createBodyOfOp<mlir::omp::WsLoopOp>(wsLoopOp, converter, loc, *nestedEval,
/*genNested=*/true, &beginClauseList, iv,
/*outer=*/false, &dsp);
createBodyOfOp<mlir::omp::WsLoopOp>(wsLoopOp,
{converter, loc, *nestedEval,
/*genNested=*/true, &beginClauseList, iv,
/*outerCombined=*/false, &dsp});
}

static void createSimdWsLoop(
Expand Down Expand Up @@ -3616,8 +3628,8 @@ genOMP(Fortran::lower::AbstractConverter &converter,
currentLocation, mlir::FlatSymbolRefAttr::get(firOpBuilder.getContext(),
global.getSymName()));
}();
createBodyOfOp<mlir::omp::CriticalOp>(criticalOp, converter, currentLocation,
eval, /*genNested=*/true);
createBodyOfOp<mlir::omp::CriticalOp>(criticalOp,
{converter, currentLocation, eval});
}

static void
Expand Down Expand Up @@ -3659,10 +3671,8 @@ genOMP(Fortran::lower::AbstractConverter &converter,
}

// SECTIONS construct
genOpWithBody<mlir::omp::SectionsOp>(converter, eval,
/*genNested=*/false, currentLocation,
/*outerCombined=*/false,
/*clauseList=*/nullptr,
genOpWithBody<mlir::omp::SectionsOp>({converter, eval,
/*genNested=*/false, currentLocation},
/*reduction_vars=*/mlir::ValueRange(),
/*reductions=*/nullptr, allocateOperands,
allocatorOperands, nowaitClauseOperand);
Expand Down