[NFC][flang][OpenMP] Split DataSharing and Clause processors
#81973
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
ergawy
requested review from
skatrak,
kiranchandramohan,
mjklemm and
kparzysz
llvmbot
added
flang
|
@llvm/pr-subscribers-flang-fir-hlfir @llvm/pr-subscribers-flang-openmp Author: Kareem Ergawy (ergawy) ChangesThis started as an experiment to reduce the compilation time of iterating over This resulted is a slightly better orgnaization of the OpenMP lowering code and hence opening this NFC. As for the compilation time, this unfortunately does not affect it much (it shaves off a few seconds of This is just a proposal, please let me know if you disagree with this reorganization for any reason.Patch is 183.18 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/81973.diff 8 Files Affected:
diff --git a/flang/lib/Lower/CMakeLists.txt b/flang/lib/Lower/CMakeLists.txt
index b13d415e02f1d9..dfd942ac21aeb1 100644
--- a/flang/lib/Lower/CMakeLists.txt
+++ b/flang/lib/Lower/CMakeLists.txt
@@ -5,6 +5,7 @@ add_flang_library(FortranLower
Allocatable.cpp
Bridge.cpp
CallInterface.cpp
+ ClauseProcessor.cpp
Coarray.cpp
ComponentPath.cpp
ConvertArrayConstructor.cpp
@@ -16,6 +17,7 @@ add_flang_library(FortranLower
ConvertType.cpp
ConvertVariable.cpp
CustomIntrinsicCall.cpp
+ DataSharingProcessor.cpp
DumpEvaluateExpr.cpp
HlfirIntrinsics.cpp
HostAssociations.cpp
@@ -25,6 +27,7 @@ add_flang_library(FortranLower
Mangler.cpp
OpenACC.cpp
OpenMP.cpp
+ OpenMPUtils.cpp
PFTBuilder.cpp
Runtime.cpp
SymbolMap.cpp
diff --git a/flang/lib/Lower/ClauseProcessor.cpp b/flang/lib/Lower/ClauseProcessor.cpp
new file mode 100644
index 00000000000000..b5c5586fa1e3ea
--- /dev/null
+++ b/flang/lib/Lower/ClauseProcessor.cpp
@@ -0,0 +1,1298 @@
+//===-- ClauseProcessor.cpp -------------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// Coding style: https://mlir.llvm.org/getting_started/DeveloperGuide/
+//
+//===----------------------------------------------------------------------===//
+
+#include "ClauseProcessor.h"
+
+#include "flang/Lower/PFTBuilder.h"
+#include "flang/Parser/tools.h"
+#include "flang/Semantics/tools.h"
+
+namespace Fortran {
+namespace lower {
+namespace omp {
+
+//===----------------------------------------------------------------------===//
+// Common helper functions
+//===----------------------------------------------------------------------===//
+
+void genObjectList(const Fortran::parser::OmpObjectList &objectList,
+ Fortran::lower::AbstractConverter &converter,
+ llvm::SmallVectorImpl<mlir::Value> &operands) {
+ auto addOperands = [&](Fortran::lower::SymbolRef sym) {
+ const mlir::Value variable = converter.getSymbolAddress(sym);
+ if (variable) {
+ operands.push_back(variable);
+ } else {
+ if (const auto *details =
+ sym->detailsIf<Fortran::semantics::HostAssocDetails>()) {
+ operands.push_back(converter.getSymbolAddress(details->symbol()));
+ converter.copySymbolBinding(details->symbol(), sym);
+ }
+ }
+ };
+ for (const Fortran::parser::OmpObject &ompObject : objectList.v) {
+ Fortran::semantics::Symbol *sym = getOmpObjectSymbol(ompObject);
+ addOperands(*sym);
+ }
+}
+
+void gatherFuncAndVarSyms(
+ const Fortran::parser::OmpObjectList &objList,
+ mlir::omp::DeclareTargetCaptureClause clause,
+ llvm::SmallVectorImpl<DeclareTargetCapturePair> &symbolAndClause) {
+ for (const Fortran::parser::OmpObject &ompObject : objList.v) {
+ Fortran::common::visit(
+ Fortran::common::visitors{
+ [&](const Fortran::parser::Designator &designator) {
+ if (const Fortran::parser::Name *name =
+ Fortran::semantics::getDesignatorNameIfDataRef(
+ designator)) {
+ symbolAndClause.emplace_back(clause, *name->symbol);
+ }
+ },
+ [&](const Fortran::parser::Name &name) {
+ symbolAndClause.emplace_back(clause, *name.symbol);
+ }},
+ ompObject.u);
+ }
+}
+
+//===----------------------------------------------------------------------===//
+// ClauseProcessor helper functions
+//===----------------------------------------------------------------------===//
+
+/// Check for unsupported map operand types.
+static void checkMapType(mlir::Location location, mlir::Type type) {
+ if (auto refType = type.dyn_cast<fir::ReferenceType>())
+ type = refType.getElementType();
+ if (auto boxType = type.dyn_cast_or_null<fir::BoxType>())
+ if (!boxType.getElementType().isa<fir::PointerType>())
+ TODO(location, "OMPD_target_data MapOperand BoxType");
+}
+
+static mlir::omp::ScheduleModifier
+translateScheduleModifier(const Fortran::parser::OmpScheduleModifierType &m) {
+ switch (m.v) {
+ case Fortran::parser::OmpScheduleModifierType::ModType::Monotonic:
+ return mlir::omp::ScheduleModifier::monotonic;
+ case Fortran::parser::OmpScheduleModifierType::ModType::Nonmonotonic:
+ return mlir::omp::ScheduleModifier::nonmonotonic;
+ case Fortran::parser::OmpScheduleModifierType::ModType::Simd:
+ return mlir::omp::ScheduleModifier::simd;
+ }
+ return mlir::omp::ScheduleModifier::none;
+}
+
+static mlir::omp::ScheduleModifier
+getScheduleModifier(const Fortran::parser::OmpScheduleClause &x) {
+ const auto &modifier =
+ std::get<std::optional<Fortran::parser::OmpScheduleModifier>>(x.t);
+ // The input may have the modifier any order, so we look for one that isn't
+ // SIMD. If modifier is not set at all, fall down to the bottom and return
+ // "none".
+ if (modifier) {
+ const auto &modType1 =
+ std::get<Fortran::parser::OmpScheduleModifier::Modifier1>(modifier->t);
+ if (modType1.v.v ==
+ Fortran::parser::OmpScheduleModifierType::ModType::Simd) {
+ const auto &modType2 = std::get<
+ std::optional<Fortran::parser::OmpScheduleModifier::Modifier2>>(
+ modifier->t);
+ if (modType2 &&
+ modType2->v.v !=
+ Fortran::parser::OmpScheduleModifierType::ModType::Simd)
+ return translateScheduleModifier(modType2->v);
+
+ return mlir::omp::ScheduleModifier::none;
+ }
+
+ return translateScheduleModifier(modType1.v);
+ }
+ return mlir::omp::ScheduleModifier::none;
+}
+
+static mlir::omp::ScheduleModifier
+getSimdModifier(const Fortran::parser::OmpScheduleClause &x) {
+ const auto &modifier =
+ std::get<std::optional<Fortran::parser::OmpScheduleModifier>>(x.t);
+ // Either of the two possible modifiers in the input can be the SIMD modifier,
+ // so look in either one, and return simd if we find one. Not found = return
+ // "none".
+ if (modifier) {
+ const auto &modType1 =
+ std::get<Fortran::parser::OmpScheduleModifier::Modifier1>(modifier->t);
+ if (modType1.v.v == Fortran::parser::OmpScheduleModifierType::ModType::Simd)
+ return mlir::omp::ScheduleModifier::simd;
+
+ const auto &modType2 = std::get<
+ std::optional<Fortran::parser::OmpScheduleModifier::Modifier2>>(
+ modifier->t);
+ if (modType2 && modType2->v.v ==
+ Fortran::parser::OmpScheduleModifierType::ModType::Simd)
+ return mlir::omp::ScheduleModifier::simd;
+ }
+ return mlir::omp::ScheduleModifier::none;
+}
+
+static void
+genAllocateClause(Fortran::lower::AbstractConverter &converter,
+ const Fortran::parser::OmpAllocateClause &ompAllocateClause,
+ llvm::SmallVectorImpl<mlir::Value> &allocatorOperands,
+ llvm::SmallVectorImpl<mlir::Value> &allocateOperands) {
+ fir::FirOpBuilder &firOpBuilder = converter.getFirOpBuilder();
+ mlir::Location currentLocation = converter.getCurrentLocation();
+ Fortran::lower::StatementContext stmtCtx;
+
+ mlir::Value allocatorOperand;
+ const Fortran::parser::OmpObjectList &ompObjectList =
+ std::get<Fortran::parser::OmpObjectList>(ompAllocateClause.t);
+ const auto &allocateModifier = std::get<
+ std::optional<Fortran::parser::OmpAllocateClause::AllocateModifier>>(
+ ompAllocateClause.t);
+
+ // If the allocate modifier is present, check if we only use the allocator
+ // submodifier. ALIGN in this context is unimplemented
+ const bool onlyAllocator =
+ allocateModifier &&
+ std::holds_alternative<
+ Fortran::parser::OmpAllocateClause::AllocateModifier::Allocator>(
+ allocateModifier->u);
+
+ if (allocateModifier && !onlyAllocator) {
+ TODO(currentLocation, "OmpAllocateClause ALIGN modifier");
+ }
+
+ // Check if allocate clause has allocator specified. If so, add it
+ // to list of allocators, otherwise, add default allocator to
+ // list of allocators.
+ if (onlyAllocator) {
+ const auto &allocatorValue = std::get<
+ Fortran::parser::OmpAllocateClause::AllocateModifier::Allocator>(
+ allocateModifier->u);
+ allocatorOperand = fir::getBase(converter.genExprValue(
+ *Fortran::semantics::GetExpr(allocatorValue.v), stmtCtx));
+ allocatorOperands.insert(allocatorOperands.end(), ompObjectList.v.size(),
+ allocatorOperand);
+ } else {
+ allocatorOperand = firOpBuilder.createIntegerConstant(
+ currentLocation, firOpBuilder.getI32Type(), 1);
+ allocatorOperands.insert(allocatorOperands.end(), ompObjectList.v.size(),
+ allocatorOperand);
+ }
+ genObjectList(ompObjectList, converter, allocateOperands);
+}
+
+static mlir::omp::ClauseProcBindKindAttr genProcBindKindAttr(
+ fir::FirOpBuilder &firOpBuilder,
+ const Fortran::parser::OmpClause::ProcBind *procBindClause) {
+ mlir::omp::ClauseProcBindKind procBindKind;
+ switch (procBindClause->v.v) {
+ case Fortran::parser::OmpProcBindClause::Type::Master:
+ procBindKind = mlir::omp::ClauseProcBindKind::Master;
+ break;
+ case Fortran::parser::OmpProcBindClause::Type::Close:
+ procBindKind = mlir::omp::ClauseProcBindKind::Close;
+ break;
+ case Fortran::parser::OmpProcBindClause::Type::Spread:
+ procBindKind = mlir::omp::ClauseProcBindKind::Spread;
+ break;
+ case Fortran::parser::OmpProcBindClause::Type::Primary:
+ procBindKind = mlir::omp::ClauseProcBindKind::Primary;
+ break;
+ }
+ return mlir::omp::ClauseProcBindKindAttr::get(firOpBuilder.getContext(),
+ procBindKind);
+}
+
+static mlir::omp::ClauseTaskDependAttr
+genDependKindAttr(fir::FirOpBuilder &firOpBuilder,
+ const Fortran::parser::OmpClause::Depend *dependClause) {
+ mlir::omp::ClauseTaskDepend pbKind;
+ switch (
+ std::get<Fortran::parser::OmpDependenceType>(
+ std::get<Fortran::parser::OmpDependClause::InOut>(dependClause->v.u)
+ .t)
+ .v) {
+ case Fortran::parser::OmpDependenceType::Type::In:
+ pbKind = mlir::omp::ClauseTaskDepend::taskdependin;
+ break;
+ case Fortran::parser::OmpDependenceType::Type::Out:
+ pbKind = mlir::omp::ClauseTaskDepend::taskdependout;
+ break;
+ case Fortran::parser::OmpDependenceType::Type::Inout:
+ pbKind = mlir::omp::ClauseTaskDepend::taskdependinout;
+ break;
+ default:
+ llvm_unreachable("unknown parser task dependence type");
+ break;
+ }
+ return mlir::omp::ClauseTaskDependAttr::get(firOpBuilder.getContext(),
+ pbKind);
+}
+
+static mlir::Value getIfClauseOperand(
+ Fortran::lower::AbstractConverter &converter,
+ const Fortran::parser::OmpClause::If *ifClause,
+ Fortran::parser::OmpIfClause::DirectiveNameModifier directiveName,
+ mlir::Location clauseLocation) {
+ // Only consider the clause if it's intended for the given directive.
+ auto &directive = std::get<
+ std::optional<Fortran::parser::OmpIfClause::DirectiveNameModifier>>(
+ ifClause->v.t);
+ if (directive && directive.value() != directiveName)
+ return nullptr;
+
+ Fortran::lower::StatementContext stmtCtx;
+ fir::FirOpBuilder &firOpBuilder = converter.getFirOpBuilder();
+ auto &expr = std::get<Fortran::parser::ScalarLogicalExpr>(ifClause->v.t);
+ mlir::Value ifVal = fir::getBase(
+ converter.genExprValue(*Fortran::semantics::GetExpr(expr), stmtCtx));
+ return firOpBuilder.createConvert(clauseLocation, firOpBuilder.getI1Type(),
+ ifVal);
+}
+
+static void
+addUseDeviceClause(Fortran::lower::AbstractConverter &converter,
+ const Fortran::parser::OmpObjectList &useDeviceClause,
+ llvm::SmallVectorImpl<mlir::Value> &operands,
+ llvm::SmallVectorImpl<mlir::Type> &useDeviceTypes,
+ llvm::SmallVectorImpl<mlir::Location> &useDeviceLocs,
+ llvm::SmallVectorImpl<const Fortran::semantics::Symbol *>
+ &useDeviceSymbols) {
+ genObjectList(useDeviceClause, converter, operands);
+ for (mlir::Value &operand : operands) {
+ checkMapType(operand.getLoc(), operand.getType());
+ useDeviceTypes.push_back(operand.getType());
+ useDeviceLocs.push_back(operand.getLoc());
+ }
+ for (const Fortran::parser::OmpObject &ompObject : useDeviceClause.v) {
+ Fortran::semantics::Symbol *sym = getOmpObjectSymbol(ompObject);
+ useDeviceSymbols.push_back(sym);
+ }
+}
+
+//===----------------------------------------------------------------------===//
+// ClauseProcessor unique clauses
+//===----------------------------------------------------------------------===//
+
+bool ClauseProcessor::processCollapse(
+ mlir::Location currentLocation, Fortran::lower::pft::Evaluation &eval,
+ llvm::SmallVectorImpl<mlir::Value> &lowerBound,
+ llvm::SmallVectorImpl<mlir::Value> &upperBound,
+ llvm::SmallVectorImpl<mlir::Value> &step,
+ llvm::SmallVectorImpl<const Fortran::semantics::Symbol *> &iv,
+ std::size_t &loopVarTypeSize) const {
+ bool found = false;
+ fir::FirOpBuilder &firOpBuilder = converter.getFirOpBuilder();
+
+ // Collect the loops to collapse.
+ Fortran::lower::pft::Evaluation *doConstructEval =
+ &eval.getFirstNestedEvaluation();
+ if (doConstructEval->getIf<Fortran::parser::DoConstruct>()
+ ->IsDoConcurrent()) {
+ TODO(currentLocation, "Do Concurrent in Worksharing loop construct");
+ }
+
+ std::int64_t collapseValue = 1l;
+ if (auto *collapseClause = findUniqueClause<ClauseTy::Collapse>()) {
+ const auto *expr = Fortran::semantics::GetExpr(collapseClause->v);
+ collapseValue = Fortran::evaluate::ToInt64(*expr).value();
+ found = true;
+ }
+
+ loopVarTypeSize = 0;
+ do {
+ Fortran::lower::pft::Evaluation *doLoop =
+ &doConstructEval->getFirstNestedEvaluation();
+ auto *doStmt = doLoop->getIf<Fortran::parser::NonLabelDoStmt>();
+ assert(doStmt && "Expected do loop to be in the nested evaluation");
+ const auto &loopControl =
+ std::get<std::optional<Fortran::parser::LoopControl>>(doStmt->t);
+ const Fortran::parser::LoopControl::Bounds *bounds =
+ std::get_if<Fortran::parser::LoopControl::Bounds>(&loopControl->u);
+ assert(bounds && "Expected bounds for worksharing do loop");
+ Fortran::lower::StatementContext stmtCtx;
+ lowerBound.push_back(fir::getBase(converter.genExprValue(
+ *Fortran::semantics::GetExpr(bounds->lower), stmtCtx)));
+ upperBound.push_back(fir::getBase(converter.genExprValue(
+ *Fortran::semantics::GetExpr(bounds->upper), stmtCtx)));
+ if (bounds->step) {
+ step.push_back(fir::getBase(converter.genExprValue(
+ *Fortran::semantics::GetExpr(bounds->step), stmtCtx)));
+ } else { // If `step` is not present, assume it as `1`.
+ step.push_back(firOpBuilder.createIntegerConstant(
+ currentLocation, firOpBuilder.getIntegerType(32), 1));
+ }
+ iv.push_back(bounds->name.thing.symbol);
+ loopVarTypeSize = std::max(loopVarTypeSize,
+ bounds->name.thing.symbol->GetUltimate().size());
+ collapseValue--;
+ doConstructEval =
+ &*std::next(doConstructEval->getNestedEvaluations().begin());
+ } while (collapseValue > 0);
+
+ return found;
+}
+
+bool ClauseProcessor::processDefault() const {
+ if (auto *defaultClause = findUniqueClause<ClauseTy::Default>()) {
+ // Private, Firstprivate, Shared, None
+ switch (defaultClause->v.v) {
+ case Fortran::parser::OmpDefaultClause::Type::Shared:
+ case Fortran::parser::OmpDefaultClause::Type::None:
+ // Default clause with shared or none do not require any handling since
+ // Shared is the default behavior in the IR and None is only required
+ // for semantic checks.
+ break;
+ case Fortran::parser::OmpDefaultClause::Type::Private:
+ // TODO Support default(private)
+ break;
+ case Fortran::parser::OmpDefaultClause::Type::Firstprivate:
+ // TODO Support default(firstprivate)
+ break;
+ }
+ return true;
+ }
+ return false;
+}
+
+bool ClauseProcessor::processDevice(Fortran::lower::StatementContext &stmtCtx,
+ mlir::Value &result) const {
+ const Fortran::parser::CharBlock *source = nullptr;
+ if (auto *deviceClause = findUniqueClause<ClauseTy::Device>(&source)) {
+ mlir::Location clauseLocation = converter.genLocation(*source);
+ if (auto deviceModifier = std::get<
+ std::optional<Fortran::parser::OmpDeviceClause::DeviceModifier>>(
+ deviceClause->v.t)) {
+ if (deviceModifier ==
+ Fortran::parser::OmpDeviceClause::DeviceModifier::Ancestor) {
+ TODO(clauseLocation, "OMPD_target Device Modifier Ancestor");
+ }
+ }
+ if (const auto *deviceExpr = Fortran::semantics::GetExpr(
+ std::get<Fortran::parser::ScalarIntExpr>(deviceClause->v.t))) {
+ result = fir::getBase(converter.genExprValue(*deviceExpr, stmtCtx));
+ }
+ return true;
+ }
+ return false;
+}
+
+bool ClauseProcessor::processDeviceType(
+ mlir::omp::DeclareTargetDeviceType &result) const {
+ if (auto *deviceTypeClause = findUniqueClause<ClauseTy::DeviceType>()) {
+ // Case: declare target ... device_type(any | host | nohost)
+ switch (deviceTypeClause->v.v) {
+ case Fortran::parser::OmpDeviceTypeClause::Type::Nohost:
+ result = mlir::omp::DeclareTargetDeviceType::nohost;
+ break;
+ case Fortran::parser::OmpDeviceTypeClause::Type::Host:
+ result = mlir::omp::DeclareTargetDeviceType::host;
+ break;
+ case Fortran::parser::OmpDeviceTypeClause::Type::Any:
+ result = mlir::omp::DeclareTargetDeviceType::any;
+ break;
+ }
+ return true;
+ }
+ return false;
+}
+
+bool ClauseProcessor::processFinal(Fortran::lower::StatementContext &stmtCtx,
+ mlir::Value &result) const {
+ const Fortran::parser::CharBlock *source = nullptr;
+ if (auto *finalClause = findUniqueClause<ClauseTy::Final>(&source)) {
+ fir::FirOpBuilder &firOpBuilder = converter.getFirOpBuilder();
+ mlir::Location clauseLocation = converter.genLocation(*source);
+
+ mlir::Value finalVal = fir::getBase(converter.genExprValue(
+ *Fortran::semantics::GetExpr(finalClause->v), stmtCtx));
+ result = firOpBuilder.createConvert(clauseLocation,
+ firOpBuilder.getI1Type(), finalVal);
+ return true;
+ }
+ return false;
+}
+
+bool ClauseProcessor::processHint(mlir::IntegerAttr &result) const {
+ if (auto *hintClause = findUniqueClause<ClauseTy::Hint>()) {
+ fir::FirOpBuilder &firOpBuilder = converter.getFirOpBuilder();
+ const auto *expr = Fortran::semantics::GetExpr(hintClause->v);
+ int64_t hintValue = *Fortran::evaluate::ToInt64(*expr);
+ result = firOpBuilder.getI64IntegerAttr(hintValue);
+ return true;
+ }
+ return false;
+}
+
+bool ClauseProcessor::processMergeable(mlir::UnitAttr &result) const {
+ return markClauseOccurrence<ClauseTy::Mergeable>(result);
+}
+
+bool ClauseProcessor::processNowait(mlir::UnitAttr &result) const {
+ return markClauseOccurrence<ClauseTy::Nowait>(result);
+}
+
+bool ClauseProcessor::processNumTeams(Fortran::lower::StatementContext &stmtCtx,
+ mlir::Value &result) const {
+ // TODO Get lower and upper bounds for num_teams when parser is updated to
+ // accept both.
+ if (auto *numTeamsClause = findUniqueClause<ClauseTy::NumTeams>()) {
+ result = fir::getBase(converter.genExprValue(
+ *Fortran::semantics::GetExpr(numTeamsClause->v), stmtCtx));
+ return true;
+ }
+ return false;
+}
+
+bool ClauseProcessor::processNumThreads(
+ Fortran::lower::StatementContext &stmtCtx, mlir::Value &result) const {
+ if (auto *numThreadsClause = findUniqueClause<ClauseTy::NumThreads>()) {
+ // OMPIRBuilder expects `NUM_THREADS` clause as a `Value`.
+ result = fir::getBase(converter.genExprValue(
+ *Fortran::semantics::GetExpr(numThreadsClause->v), stmtCtx));
+ return true;
+ }
+ return false;
+...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
ergawy
force-pushed
the
openmp.cpp_compilation_time
branch
from
6d15e28 to
1cb98c8
Compare
|
This makes sense to me. I'd create a subdirectory |
ergawy
force-pushed
the
openmp.cpp_compilation_time
branch
from
1cb98c8 to
8d80bd6
Compare
Thanks for taking a look @kparzysz. Moved to sub-dir and also split |
ergawy
force-pushed
the
openmp.cpp_compilation_time
branch
2 times, most recently
from
1901c40 to
7549dbf
Compare
|
Ping 🔔! Can you please have a look? 🙏 |
ergawy
force-pushed
the
openmp.cpp_compilation_time
branch
from
7549dbf to
bb85bdf
Compare
skatrak
reviewed
Feb 21, 2024
Comment on lines
23
to
25
| //===----------------------------------------------------------------------===// | ||
| // Common helper functions | ||
| //===----------------------------------------------------------------------===// |
There was a problem hiding this comment.
Nit: This comment was originally intended to split up helper functions that were used in multiple places of OpenMP.cpp from other helpers that only applied to the ClauseProcessor or other components. I think it's safe to say that here every helper applies, so maybe it makes sense to rename it to "Helper functions" and remove the "ClauseProcessor helper functions" comment below.
There was a problem hiding this comment.
Moved to Utils.h, so I think there is no need for the comment there.
|
Nice work. Would we be able to move OpenMP.cpp also to the OpenMP directory? |
ergawy
force-pushed
the
openmp.cpp_compilation_time
branch
3 times, most recently
from
8d8cf3d to
b3f9dd0
Compare
We can do that. Done! |
ergawy
force-pushed
the
openmp.cpp_compilation_time
branch
from
b3f9dd0 to
d277f10
Compare
kiranchandramohan
approved these changes
Feb 21, 2024
There was a problem hiding this comment.
This will bring some short term pain for all the patches in flight. On our side particularly this will impact (refactoring cost) for the array reduction work. But overall this is a good patch and it LGTM. Please wait for other reviewers.
Please update the summary and title to include all the changes.
This started as an experiment to reduce the compilation time of iterating over `Lower/OpenMP.cpp` a bit since it is too slow at the moment. Trying to do that, I split the `DataSharingProcessor` and `ClauseProcessor` into their own files and extracted some shared code into a util file. This resulted is a slightly better orgnaization of the OpenMP lowering code and hence opening this NFC. As for the compilation time, this unfortunately does not affect it much (it shaves off a few seconds of `OpenMP.cpp` compilation) since from what I learned the bottleneck is in `DirectivesCommon.h` and `PFTBuilder.h` which both consume a lot of time in template instantiation it seems.
ergawy
force-pushed
the
openmp.cpp_compilation_time
branch
from
d277f10 to
a1fb75c
Compare
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 started as an experiment to reduce the compilation time of iterating over
Lower/OpenMP.cppa bit since it is too slow at the moment. Trying to do that, I split theDataSharingProcessor,ReductionProcessor, andClauseProcessorinto their own files and extracted some shared code into a util file. All of these new.h/.cppfiles as well asOpenMP.cppare now under aLower/OpenMP/directory.This resulted is a slightly better organization of the OpenMP lowering code and hence opening this NFC.
As for the compilation time, this unfortunately does not affect it much (it shaves off a few seconds of
OpenMP.cppcompilation) since from what I learned the bottleneck is inDirectivesCommon.handPFTBuilder.hwhich both consume a lot of time in template instantiation it seems.