Skip to content

Commit

Permalink
[Flang][OpenMP] Remove unused OpWithBodyGenInfo attributes (llvm#97572)
Browse files Browse the repository at this point in the history 
This patch removes the `outerCombined`, `reductionSymbols` and
`reductionTypes` attributes from the `OpWithBodyGenInfo` structure and
their uses, as they never impact the lowering process or its output.

The `outerCombined` variable is always set to `false`, so in practice it
doesn't represent what its name indicates. Furthermore, initializing it
correctly can result in privatization not being performed in cases where
it should (at least tests doing this together with composite construct
support pointed me in that direction). It seems to be tied to the early
privatization approach, where a redundant alloca could possibly be
avoided in certain cases. With the transition to delayed privatization,
it seems like it won't serve that purpose anymore, since the decision of
what and where privatization-related allocations are inserted will be
postponed to the MLIR to LLVM IR translation stage. Since this feature
is already currently not being used, its potential benefit appears to be
minor and it won't make sense to do once the delayed privatization
approach is rolled out, I propose removing it.

The `reductionSymbols` and `reductionTypes` variables are set in certain
cases but never used. Unless there's a plan where these will be needed,
in which case it would be a better alternative to document it, I believe
we should also remove them.
  • Loading branch information
@skatrak
skatrak committed Jul 5, 2024
1 parent 99d6c6d commit 2d0c4c3
Showing 1 changed file with 14 additions and 50 deletions.
64 changes: 14 additions & 50 deletions flang/lib/Lower/OpenMP/OpenMP.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -504,11 +504,6 @@ struct OpWithBodyGenInfo {
: converter(converter), symTable(symTable), semaCtx(semaCtx), loc(loc),
eval(eval), dir(dir) {}

OpWithBodyGenInfo &setOuterCombined(bool value) {
outerCombined = value;
return *this;
}

OpWithBodyGenInfo &setClauses(const List<Clause> *value) {
clauses = value;
return *this;
Expand All @@ -519,14 +514,6 @@ struct OpWithBodyGenInfo {
return *this;
}

OpWithBodyGenInfo &
setReductions(llvm::SmallVectorImpl<const semantics::Symbol *> *value1,
llvm::SmallVectorImpl<mlir::Type> *value2) {
reductionSymbols = value1;
reductionTypes = value2;
return *this;
}

OpWithBodyGenInfo &setGenRegionEntryCb(GenOMPRegionEntryCBFn value) {
genRegionEntryCB = value;
return *this;
Expand All @@ -544,16 +531,10 @@ struct OpWithBodyGenInfo {
lower::pft::Evaluation &eval;
/// [in] leaf directive for which to generate the op body.
llvm::omp::Directive dir;
/// [in] is this an outer operation - prevents privatization.
bool outerCombined = false;
/// [in] list of clauses to process.
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
llvm::SmallVectorImpl<const semantics::Symbol *> *reductionSymbols = nullptr;
/// [in] if provided, list of reduction types
llvm::SmallVectorImpl<mlir::Type> *reductionTypes = nullptr;
/// [in] if provided, emits the op's region entry. Otherwise, an emtpy block
/// is created in the region.
GenOMPRegionEntryCBFn genRegionEntryCB = nullptr;
Expand Down Expand Up @@ -591,26 +572,23 @@ static void createBodyOfOp(mlir::Operation &op, const OpWithBodyGenInfo &info,
// Mark the earliest insertion point.
mlir::Operation *marker = insertMarker(firOpBuilder);

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

// Start with privatization, so that the lowering of the nested
// code will use the right symbols.
bool isLoop = llvm::omp::getDirectiveAssociation(info.dir) ==
llvm::omp::Association::Loop;
bool privatize = info.clauses && !info.outerCombined;
bool privatize = info.clauses;

firOpBuilder.setInsertionPoint(marker);
std::optional<DataSharingProcessor> tempDsp;
if (privatize) {
if (!info.dsp) {
tempDsp.emplace(info.converter, info.semaCtx, *info.clauses, info.eval,
Fortran::lower::omp::isLastItemInQueue(item, queue));
tempDsp->processStep1();
}
if (privatize && !info.dsp) {
tempDsp.emplace(info.converter, info.semaCtx, *info.clauses, info.eval,
Fortran::lower::omp::isLastItemInQueue(item, queue));
tempDsp->processStep1();
}

if (info.dir == llvm::omp::Directive::OMPD_parallel) {
Expand Down Expand Up @@ -1078,8 +1056,7 @@ genOrderedRegionClauses(lower::AbstractConverter &converter,
static void genParallelClauses(
lower::AbstractConverter &converter, semantics::SemanticsContext &semaCtx,
lower::StatementContext &stmtCtx, const List<Clause> &clauses,
mlir::Location loc, bool processReduction,
mlir::omp::ParallelClauseOps &clauseOps,
mlir::Location loc, mlir::omp::ParallelClauseOps &clauseOps,
llvm::SmallVectorImpl<mlir::Type> &reductionTypes,
llvm::SmallVectorImpl<const semantics::Symbol *> &reductionSyms) {
ClauseProcessor cp(converter, semaCtx, clauses);
Expand All @@ -1088,10 +1065,7 @@ static void genParallelClauses(
cp.processIf(llvm::omp::Directive::OMPD_parallel, clauseOps);
cp.processNumThreads(stmtCtx, clauseOps);
cp.processProcBind(clauseOps);

if (processReduction) {
cp.processReduction(loc, clauseOps, &reductionTypes, &reductionSyms);
}
cp.processReduction(loc, clauseOps, &reductionTypes, &reductionSyms);
}

static void genSectionsClauses(lower::AbstractConverter &converter,
Expand Down Expand Up @@ -1440,15 +1414,13 @@ static mlir::omp::ParallelOp
genParallelOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
semantics::SemanticsContext &semaCtx,
lower::pft::Evaluation &eval, mlir::Location loc,
const ConstructQueue &queue, ConstructQueue::iterator item,
bool outerCombined = false) {
const ConstructQueue &queue, ConstructQueue::iterator item) {
fir::FirOpBuilder &firOpBuilder = converter.getFirOpBuilder();
lower::StatementContext stmtCtx;
mlir::omp::ParallelClauseOps clauseOps;
llvm::SmallVector<mlir::Type> reductionTypes;
llvm::SmallVector<const semantics::Symbol *> reductionSyms;
genParallelClauses(converter, semaCtx, stmtCtx, item->clauses, loc,
/*processReduction=*/!outerCombined, clauseOps,
genParallelClauses(converter, semaCtx, stmtCtx, item->clauses, loc, clauseOps,
reductionTypes, reductionSyms);

auto reductionCallback = [&](mlir::Operation *op) {
Expand All @@ -1459,22 +1431,17 @@ genParallelOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
OpWithBodyGenInfo genInfo =
OpWithBodyGenInfo(converter, symTable, semaCtx, loc, eval,
llvm::omp::Directive::OMPD_parallel)
.setOuterCombined(outerCombined)
.setClauses(&item->clauses)
.setReductions(&reductionSyms, &reductionTypes)
.setGenRegionEntryCb(reductionCallback);

if (!enableDelayedPrivatization)
return genOpWithBody<mlir::omp::ParallelOp>(genInfo, queue, item,
clauseOps);

bool privatize = !outerCombined;
DataSharingProcessor dsp(converter, semaCtx, item->clauses, eval,
lower::omp::isLastItemInQueue(item, queue),
/*useDelayedPrivatization=*/true, &symTable);

if (privatize)
dsp.processStep1(&clauseOps);
dsp.processStep1(&clauseOps);

auto genRegionEntryCB = [&](mlir::Operation *op) {
auto parallelOp = llvm::cast<mlir::omp::ParallelOp>(op);
Expand Down Expand Up @@ -1921,15 +1888,14 @@ static mlir::omp::TeamsOp
genTeamsOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
semantics::SemanticsContext &semaCtx, lower::pft::Evaluation &eval,
mlir::Location loc, const ConstructQueue &queue,
ConstructQueue::iterator item, bool outerCombined = false) {
ConstructQueue::iterator item) {
lower::StatementContext stmtCtx;
mlir::omp::TeamsClauseOps clauseOps;
genTeamsClauses(converter, semaCtx, stmtCtx, item->clauses, loc, clauseOps);

return genOpWithBody<mlir::omp::TeamsOp>(
OpWithBodyGenInfo(converter, symTable, semaCtx, loc, eval,
llvm::omp::Directive::OMPD_teams)
.setOuterCombined(outerCombined)
.setClauses(&item->clauses),
queue, item, clauseOps);
}
Expand Down Expand Up @@ -1982,7 +1948,6 @@ genWsloopOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
*nestedEval, llvm::omp::Directive::OMPD_do)
.setClauses(&item->clauses)
.setDataSharingProcessor(&dsp)
.setReductions(&reductionSyms, &reductionTypes)
.setGenRegionEntryCb(ivCallback),
queue, item);
symTable.popScope();
Expand Down Expand Up @@ -2084,8 +2049,7 @@ static void genOMPDispatch(lower::AbstractConverter &converter,
genOrderedRegionOp(converter, symTable, semaCtx, eval, loc, queue, item);
break;
case llvm::omp::Directive::OMPD_parallel:
genParallelOp(converter, symTable, semaCtx, eval, loc, queue, item,
/*outerCombined=*/false);
genParallelOp(converter, symTable, semaCtx, eval, loc, queue, item);
break;
case llvm::omp::Directive::OMPD_section:
genSectionOp(converter, symTable, semaCtx, eval, loc, queue, item);
Expand Down

0 comments on commit 2d0c4c3

Please sign in to comment.