-
Notifications
You must be signed in to change notification settings - Fork 15.1k
[NFC][flang][OpenMP] Create FortranUtils
lib and move createMapInfoOp
to it
#154483
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.
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
Conversation
@llvm/pr-subscribers-flang-fir-hlfir Author: Kareem Ergawy (ergawy) ChangesThis moves Context: I am working on upstreaming (from AMD's downstream fork) support for The issue now is that we have to link in both the FIR and OpenMP MLIR dialects which is not ideal to link with a suport library like Note that, so far, upstream Full diff: https://github.com/llvm/llvm-project/pull/154483.diff 6 Files Affected:
diff --git a/flang/include/flang/Support/OpenMP-utils.h b/flang/include/flang/Support/OpenMP-utils.h
index 6d9db2b682c50..88b30c376de85 100644
--- a/flang/include/flang/Support/OpenMP-utils.h
+++ b/flang/include/flang/Support/OpenMP-utils.h
@@ -11,6 +11,7 @@
#include "flang/Semantics/symbol.h"
+#include "mlir/Dialect/OpenMP/OpenMPDialect.h"
#include "mlir/IR/Builders.h"
#include "mlir/IR/Value.h"
@@ -72,6 +73,14 @@ struct EntryBlockArgs {
/// \param [in] region - Empty region in which to create the entry block.
mlir::Block *genEntryBlock(
mlir::OpBuilder &builder, const EntryBlockArgs &args, mlir::Region ®ion);
+
+mlir::omp::MapInfoOp createMapInfoOp(mlir::OpBuilder &builder,
+ mlir::Location loc, mlir::Value baseAddr, mlir::Value varPtrPtr,
+ llvm::StringRef name, llvm::ArrayRef<mlir::Value> bounds,
+ llvm::ArrayRef<mlir::Value> members, mlir::ArrayAttr membersIndex,
+ uint64_t mapType, mlir::omp::VariableCaptureKind mapCaptureType,
+ mlir::Type retTy, bool partialMap = false,
+ mlir::FlatSymbolRefAttr mapperId = mlir::FlatSymbolRefAttr());
} // namespace Fortran::common::openmp
#endif // FORTRAN_SUPPORT_OPENMP_UTILS_H_
diff --git a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
index 5d19f589d79fc..bf4b66d70e70b 100644
--- a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
+++ b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
@@ -19,6 +19,7 @@
#include "flang/Lower/Support/ReductionProcessor.h"
#include "flang/Parser/tools.h"
#include "flang/Semantics/tools.h"
+#include "flang/Support/OpenMP-utils.h"
#include "llvm/Frontend/OpenMP/OMP.h.inc"
#include "llvm/Frontend/OpenMP/OMPIRBuilder.h"
@@ -1281,7 +1282,7 @@ void ClauseProcessor::processMapObjects(
auto location = mlir::NameLoc::get(
mlir::StringAttr::get(firOpBuilder.getContext(), asFortran.str()),
baseOp.getLoc());
- mlir::omp::MapInfoOp mapOp = createMapInfoOp(
+ mlir::omp::MapInfoOp mapOp = common::openmp::createMapInfoOp(
firOpBuilder, location, baseOp,
/*varPtrPtr=*/mlir::Value{}, asFortran.str(), bounds,
/*members=*/{}, /*membersIndex=*/mlir::ArrayAttr{},
diff --git a/flang/lib/Lower/OpenMP/Utils.cpp b/flang/lib/Lower/OpenMP/Utils.cpp
index 77b1e39083aa6..e9c13deb9aae7 100644
--- a/flang/lib/Lower/OpenMP/Utils.cpp
+++ b/flang/lib/Lower/OpenMP/Utils.cpp
@@ -24,6 +24,7 @@
#include <flang/Parser/parse-tree.h>
#include <flang/Parser/tools.h>
#include <flang/Semantics/tools.h>
+#include <flang/Support/OpenMP-utils.h>
#include <llvm/Support/CommandLine.h>
#include <iterator>
@@ -108,38 +109,6 @@ void gatherFuncAndVarSyms(
symbolAndClause.emplace_back(clause, *object.sym(), automap);
}
-mlir::omp::MapInfoOp
-createMapInfoOp(fir::FirOpBuilder &builder, mlir::Location loc,
- mlir::Value baseAddr, mlir::Value varPtrPtr,
- llvm::StringRef name, llvm::ArrayRef<mlir::Value> bounds,
- llvm::ArrayRef<mlir::Value> members,
- mlir::ArrayAttr membersIndex, uint64_t mapType,
- mlir::omp::VariableCaptureKind mapCaptureType, mlir::Type retTy,
- bool partialMap, mlir::FlatSymbolRefAttr mapperId) {
- if (auto boxTy = llvm::dyn_cast<fir::BaseBoxType>(baseAddr.getType())) {
- baseAddr = fir::BoxAddrOp::create(builder, loc, baseAddr);
- retTy = baseAddr.getType();
- }
-
- mlir::TypeAttr varType = mlir::TypeAttr::get(
- llvm::cast<mlir::omp::PointerLikeType>(retTy).getElementType());
-
- // For types with unknown extents such as <2x?xi32> we discard the incomplete
- // type info and only retain the base type. The correct dimensions are later
- // recovered through the bounds info.
- if (auto seqType = llvm::dyn_cast<fir::SequenceType>(varType.getValue()))
- if (seqType.hasDynamicExtents())
- varType = mlir::TypeAttr::get(seqType.getEleTy());
-
- mlir::omp::MapInfoOp op = mlir::omp::MapInfoOp::create(
- builder, loc, retTy, baseAddr, varType,
- builder.getIntegerAttr(builder.getIntegerType(64, false), mapType),
- builder.getAttr<mlir::omp::VariableCaptureKindAttr>(mapCaptureType),
- varPtrPtr, members, membersIndex, bounds, mapperId,
- builder.getStringAttr(name), builder.getBoolAttr(partialMap));
- return op;
-}
-
// This function gathers the individual omp::Object's that make up a
// larger omp::Object symbol.
//
@@ -403,7 +372,7 @@ mlir::Value createParentSymAndGenIntermediateMaps(
// Create a map for the intermediate member and insert it and it's
// indices into the parentMemberIndices list to track it.
- mlir::omp::MapInfoOp mapOp = createMapInfoOp(
+ mlir::omp::MapInfoOp mapOp = common::openmp::createMapInfoOp(
firOpBuilder, clauseLocation, curValue,
/*varPtrPtr=*/mlir::Value{}, asFortran,
/*bounds=*/interimBounds,
@@ -563,7 +532,7 @@ void insertChildMapInfoIntoParent(
converter.getCurrentLocation(), asFortran, bounds,
treatIndexAsSection);
- mlir::omp::MapInfoOp mapOp = createMapInfoOp(
+ mlir::omp::MapInfoOp mapOp = common::openmp::createMapInfoOp(
firOpBuilder, info.rawInput.getLoc(), info.rawInput,
/*varPtrPtr=*/mlir::Value(), asFortran.str(), bounds, members,
firOpBuilder.create2DI64ArrayAttr(
diff --git a/flang/lib/Lower/OpenMP/Utils.h b/flang/lib/Lower/OpenMP/Utils.h
index 60f44a7f0610c..88371ab8bf969 100644
--- a/flang/lib/Lower/OpenMP/Utils.h
+++ b/flang/lib/Lower/OpenMP/Utils.h
@@ -114,16 +114,6 @@ struct OmpMapParentAndMemberData {
semantics::SemanticsContext &semaCtx);
};
-mlir::omp::MapInfoOp
-createMapInfoOp(fir::FirOpBuilder &builder, mlir::Location loc,
- mlir::Value baseAddr, mlir::Value varPtrPtr,
- llvm::StringRef name, llvm::ArrayRef<mlir::Value> bounds,
- llvm::ArrayRef<mlir::Value> members,
- mlir::ArrayAttr membersIndex, uint64_t mapType,
- mlir::omp::VariableCaptureKind mapCaptureType, mlir::Type retTy,
- bool partialMap = false,
- mlir::FlatSymbolRefAttr mapperId = mlir::FlatSymbolRefAttr());
-
void insertChildMapInfoIntoParent(
Fortran::lower::AbstractConverter &converter,
Fortran::semantics::SemanticsContext &semaCtx,
diff --git a/flang/lib/Support/CMakeLists.txt b/flang/lib/Support/CMakeLists.txt
index 363f57ce97dae..b696851fd23ab 100644
--- a/flang/lib/Support/CMakeLists.txt
+++ b/flang/lib/Support/CMakeLists.txt
@@ -54,10 +54,17 @@ add_flang_library(FortranSupport
Version.cpp
${version_inc}
+ DEPENDS
+ FIRDialect
+
+ LINK_LIBS
+ FIRDialect
+
LINK_COMPONENTS
Support
MLIR_LIBS
MLIRIR
MLIRSupport
+ MLIROpenMPDialect
)
diff --git a/flang/lib/Support/OpenMP-utils.cpp b/flang/lib/Support/OpenMP-utils.cpp
index 97e7723f0be8c..b33325042df5b 100644
--- a/flang/lib/Support/OpenMP-utils.cpp
+++ b/flang/lib/Support/OpenMP-utils.cpp
@@ -7,7 +7,10 @@
//===----------------------------------------------------------------------===//
#include "flang/Support/OpenMP-utils.h"
+#include "flang/Optimizer/Dialect/FIROps.h"
+#include "flang/Optimizer/Dialect/FIRType.h"
+#include "mlir/Dialect/OpenMP/OpenMPDialect.h"
#include "mlir/IR/OpDefinition.h"
namespace Fortran::common::openmp {
@@ -47,4 +50,35 @@ mlir::Block *genEntryBlock(mlir::OpBuilder &builder, const EntryBlockArgs &args,
return builder.createBlock(®ion, {}, types, locs);
}
+
+mlir::omp::MapInfoOp createMapInfoOp(mlir::OpBuilder &builder,
+ mlir::Location loc, mlir::Value baseAddr, mlir::Value varPtrPtr,
+ llvm::StringRef name, llvm::ArrayRef<mlir::Value> bounds,
+ llvm::ArrayRef<mlir::Value> members, mlir::ArrayAttr membersIndex,
+ uint64_t mapType, mlir::omp::VariableCaptureKind mapCaptureType,
+ mlir::Type retTy, bool partialMap, mlir::FlatSymbolRefAttr mapperId) {
+
+ if (auto boxTy = llvm::dyn_cast<fir::BaseBoxType>(baseAddr.getType())) {
+ baseAddr = fir::BoxAddrOp::create(builder, loc, baseAddr);
+ retTy = baseAddr.getType();
+ }
+
+ mlir::TypeAttr varType = mlir::TypeAttr::get(
+ llvm::cast<mlir::omp::PointerLikeType>(retTy).getElementType());
+
+ // For types with unknown extents such as <2x?xi32> we discard the incomplete
+ // type info and only retain the base type. The correct dimensions are later
+ // recovered through the bounds info.
+ if (auto seqType = llvm::dyn_cast<fir::SequenceType>(varType.getValue()))
+ if (seqType.hasDynamicExtents())
+ varType = mlir::TypeAttr::get(seqType.getEleTy());
+
+ mlir::omp::MapInfoOp op =
+ mlir::omp::MapInfoOp::create(builder, loc, retTy, baseAddr, varType,
+ builder.getIntegerAttr(builder.getIntegerType(64, false), mapType),
+ builder.getAttr<mlir::omp::VariableCaptureKindAttr>(mapCaptureType),
+ varPtrPtr, members, membersIndex, bounds, mapperId,
+ builder.getStringAttr(name), builder.getBoolAttr(partialMap));
+ return op;
+}
} // namespace Fortran::common::openmp
|
As mentioned in the PR description, this might be a good solution. However, I opened the PR for discussion and happy to receive any feedback or suggestions. |
mlir::Block *genEntryBlock( | ||
mlir::OpBuilder &builder, const EntryBlockArgs &args, mlir::Region ®ion); | ||
|
||
mlir::omp::MapInfoOp createMapInfoOp(mlir::OpBuilder &builder, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding some ///
documentation
Since the PR is for discussion, I am not LGTM it yet. Consider adding NFC to the title. |
createMapInfoOp
to OpenMP-utils.h
createMapInfoOp
to OpenMP-utils.h
@Meinersbur Thanks for the review. Added docs. Also added the |
Ping! Please take a look and let me know if you have objections to changes in this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is okay with me so long as you're sure the new dependencies in fir/lib/Support/CMakeLists.txt do not introduce circular dependencies. I presume any circular dependencies would have already broken builds for your downstream?
So I guess the important change is adding a dependency on FIRDialect to flang/Support. So far as I can tell this looks safe.
This has been downstream for 3 months now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving this to shared place seems to make sense to avoid code duplication.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we have MapInfoFinalization
, flang/lib/Optimizer/OpenMP/MapsForPrivatizedSymbols.cpp, etc make use of this? Having a single PR to refactor all of these seems like a good idea to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, please do make sure this builds with -DBUILD_SHARED_LIBS=ON and OFF! Just recalling some minor issues from this refactor downstream that'd be good to avoid repeating upstream :-)
I will second this and will request that build with |
My opinion is that this is not the right change from library layering perspective. A |
b9a7958
to
518312a
Compare
Tried both with an empty build folder and builds and lit tests work fine. |
Potentially |
Thanks for the pointer @Meinersbur. So, if we follow the same route, we would add a new lib (e.g. However, tbh I don't see much value in that because:
But I might have misunderstood what Abid means here. |
I think there is some usefulness. "Support" accross LLVM projects denotes general purpose utilities that could have been part of a C++ standard library, i.e. are not even specific to a compiler project1. IMHO e.g. Footnotes
|
This moves `createMapInfoOp` from `flang/lib/Lower/OpenMP/Utils.h` to `flang/include/flang/Support/OpenMP-utils.h`. Context: I am working on upstreaming (from AMD's downstream fork) support for `do concurrent` mapping to the GPU. This means that `DoConcurrentConversion.cpp` needs access to `createMapInfoOp`. Hence, we moved it downstream to a shared location between that is linked by both `FlangOpenMPTransforms` (where the `do concurrent` pass lives) and `FortranLower` (where `createMapInfoOp` originally were). The issue now is that we have to link in both the FIR and OpenMP MLIR dialects which is not ideal to link with a suport library like `FortranSupport`. Note that, so far, upstream `DoConcurrentConversion.cpp` does not reference `createMapInfoOp` yet. Follow-up PRs will upstream this later.
518312a
to
ed16ebe
Compare
ed16ebe
to
f9a4057
Compare
Thanks for the context. I added a new lib and moved the function to it. |
createMapInfoOp
to OpenMP-utils.h
FortranUtils
lib and move createMapInfoOp
to it
Since these are quite different (i.e. we cannot reuse |
Hi @ergawy Thanks for working on my comments and thanks to @Meinersbur for explaining issues with Another way to understand my earlier comment is to look at the include headers in |
I will investigate this failure. |
It seems that it got fixed in #155422. |
Thanks for looking into the linking failure. Let me know if we can merge the PR now or still have other comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on it. LGTM.
…oOp` to it (llvm#154483) Applies upstream PR llvm#154483. This moves `createMapInfoOp` from `flang/lib/Lower/OpenMP/Utils.h` to new utils library. Context: I am working on upstreaming (from AMD's downstream fork) support for `do concurrent` mapping to the GPU. This means that `DoConcurrentConversion.cpp` needs access to `createMapInfoOp`. Hence, we moved it downstream to a shared location between that is linked by both `FlangOpenMPTransforms` (where the `do concurrent` pass lives) and `FortranLower` (where `createMapInfoOp` originally were). Note that, so far, upstream `DoConcurrentConversion.cpp` does not reference `createMapInfoOp` yet. Follow-up PRs will upstream this later.
…ide values Following up on #154483, this PR introduces further refactoring to extract some shared utils between OpenMP lowering and `do concurrent` conversion pass. In particular, this PR extracts 2 utils that handle mapping or cloning values used inside target regions but defined outside. Later `do concurrent` PR(s) will use these utils.
…ide values Following up on #154483, this PR introduces further refactoring to extract some shared utils between OpenMP lowering and `do concurrent` conversion pass. In particular, this PR extracts 2 utils that handle mapping or cloning values used inside target regions but defined outside. Later `do concurrent` PR(s) will use these utils.
…ide values Following up on #154483, this PR introduces further refactoring to extract some shared utils between OpenMP lowering and `do concurrent` conversion pass. In particular, this PR extracts 2 utils that handle mapping or cloning values used inside target regions but defined outside. Later `do concurrent` PR(s) will use these utils.
…ide values Following up on #154483, this PR introduces further refactoring to extract some shared utils between OpenMP lowering and `do concurrent` conversion pass. In particular, this PR extracts 2 utils that handle mapping or cloning values used inside target regions but defined outside. Later `do concurrent` PR(s) will use these utils.
…ide values Following up on #154483, this PR introduces further refactoring to extract some shared utils between OpenMP lowering and `do concurrent` conversion pass. In particular, this PR extracts 2 utils that handle mapping or cloning values used inside target regions but defined outside. Later `do concurrent` PR(s) will use these utils.
…ide values Following up on #154483, this PR introduces further refactoring to extract some shared utils between OpenMP lowering and `do concurrent` conversion pass. In particular, this PR extracts 2 utils that handle mapping or cloning values used inside target regions but defined outside. Later `do concurrent` PR(s) will use these utils.
…ide values (#155754) Following up on #154483, this PR introduces further refactoring to extract some shared utils between OpenMP lowering and `do concurrent` conversion pass. In particular, this PR extracts 2 utils that handle mapping or cloning values used inside target regions but defined outside. Later `do concurrent` PR(s) will also use these utils. PR stack: - #155754◀️ - #155987 - #155992 - #155993 - #156589 - #156610 - #156837
This moves
createMapInfoOp
fromflang/lib/Lower/OpenMP/Utils.h
to new utils library.Context: I am working on upstreaming (from AMD's downstream fork) support for
do concurrent
mapping to the GPU. This means thatDoConcurrentConversion.cpp
needs access tocreateMapInfoOp
. Hence, we moved it downstream to a shared location between that is linked by bothFlangOpenMPTransforms
(where thedo concurrent
pass lives) andFortranLower
(wherecreateMapInfoOp
originally were).Note that, so far, upstream
DoConcurrentConversion.cpp
does not referencecreateMapInfoOp
yet. Follow-up PRs will upstream this later.