[Flang][OpenMP] Remove unused OpWithBodyGenInfo attributes #97572
Conversation
This file contains 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
skatrak
requested review from
ergawy,
Meinersbur,
jsjodin,
tblah,
kiranchandramohan,
abidh,
mjklemm,
raghavendhra,
agozillon,
DominikAdamski,
TIFitis,
kparzysz,
anchuraj and
bhandarkar-pranav
llvmbot
added
flang
|
@llvm/pr-subscribers-flang-fir-hlfir Author: Sergio Afonso (skatrak) ChangesThis patch removes the The The Full diff: https://github.com/llvm/llvm-project/pull/97572.diff 1 Files Affected:
diff --git a/flang/lib/Lower/OpenMP/OpenMP.cpp b/flang/lib/Lower/OpenMP/OpenMP.cpp
index 9ad092a1a00bd..e1efe0d1bd2d4 100644
--- a/flang/lib/Lower/OpenMP/OpenMP.cpp
+++ b/flang/lib/Lower/OpenMP/OpenMP.cpp
@@ -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;
@@ -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;
@@ -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;
@@ -591,9 +572,8 @@ 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());
@@ -601,16 +581,14 @@ static void createBodyOfOp(mlir::Operation &op, const OpWithBodyGenInfo &info,
// 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) {
@@ -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);
@@ -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,
@@ -1136,7 +1110,7 @@ static void genSingleClauses(lower::AbstractConverter &converter,
static void genTargetClauses(
lower::AbstractConverter &converter, semantics::SemanticsContext &semaCtx,
lower::StatementContext &stmtCtx, const List<Clause> &clauses,
- mlir::Location loc, bool processHostOnlyClauses, bool processReduction,
+ mlir::Location loc, bool processHostOnlyClauses,
mlir::omp::TargetClauseOps &clauseOps,
llvm::SmallVectorImpl<const semantics::Symbol *> &mapSyms,
llvm::SmallVectorImpl<mlir::Location> &mapLocs,
@@ -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) {
@@ -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);
@@ -1678,7 +1645,7 @@ static mlir::omp::TargetOp
genTargetOp(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) {
fir::FirOpBuilder &firOpBuilder = converter.getFirOpBuilder();
lower::StatementContext stmtCtx;
@@ -1692,10 +1659,9 @@ genTargetOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
llvm::SmallVector<mlir::Location> mapLocs, devicePtrLocs, deviceAddrLocs;
llvm::SmallVector<mlir::Type> mapTypes, devicePtrTypes, deviceAddrTypes;
genTargetClauses(converter, semaCtx, stmtCtx, item->clauses, loc,
- processHostOnlyClauses, /*processReduction=*/outerCombined,
- clauseOps, mapSyms, mapLocs, mapTypes, deviceAddrSyms,
- deviceAddrLocs, deviceAddrTypes, devicePtrSyms,
- devicePtrLocs, devicePtrTypes);
+ processHostOnlyClauses, clauseOps, mapSyms, mapLocs,
+ mapTypes, deviceAddrSyms, deviceAddrLocs, deviceAddrTypes,
+ devicePtrSyms, devicePtrLocs, devicePtrTypes);
llvm::SmallVector<const semantics::Symbol *> privateSyms;
DataSharingProcessor dsp(converter, semaCtx, item->clauses, eval,
@@ -1922,7 +1888,7 @@ 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);
@@ -1930,7 +1896,6 @@ genTeamsOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
return genOpWithBody<mlir::omp::TeamsOp>(
OpWithBodyGenInfo(converter, symTable, semaCtx, loc, eval,
llvm::omp::Directive::OMPD_teams)
- .setOuterCombined(outerCombined)
.setClauses(&item->clauses),
queue, item, clauseOps);
}
@@ -1983,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();
@@ -2085,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);
@@ -2101,8 +2064,7 @@ static void genOMPDispatch(lower::AbstractConverter &converter,
genSingleOp(converter, symTable, semaCtx, eval, loc, queue, item);
break;
case llvm::omp::Directive::OMPD_target:
- genTargetOp(converter, symTable, semaCtx, eval, loc, queue, item,
- /*outerCombined=*/false);
+ genTargetOp(converter, symTable, semaCtx, eval, loc, queue, item);
break;
case llvm::omp::Directive::OMPD_target_data:
genTargetDataOp(converter, symTable, semaCtx, eval, loc, queue, item);
|
tblah
approved these changes
Jul 4, 2024
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.
skatrak
force-pushed
the
unused-opwithbodygeninfo
branch
from
0b63f5f
to
7439c9a
Compare
kbluck
pushed a commit
to kbluck/llvm-project
that referenced
this pull request
Jul 6, 2024
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.
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 patch removes the
outerCombined,reductionSymbolsandreductionTypesattributes from theOpWithBodyGenInfostructure and their uses, as they never impact the lowering process or its output.The
outerCombinedvariable is always set tofalse, 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
reductionSymbolsandreductionTypesvariables 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.