[flang][OpenMP] Adapt OMPMapInfoFinalization to run on all top level ops #93545
Conversation
This is generally just for consistency with the rest of the pipeline. The assertion for the insertion point is because I am not sure if omp::PrivateClauseOp is supported by FirOpBuilder::getAllocaBlock. I dind't try to fix it because I don't see why we would generate IR like that. See RFC: https://discourse.llvm.org/t/rfc-add-an-interface-for-top-level-container-operations
llvmbot
added
flang
|
@llvm/pr-subscribers-flang-fir-hlfir @llvm/pr-subscribers-flang-openmp Author: Tom Eccles (tblah) ChangesThis is generally just for consistency with the rest of the pipeline. The assertion for the insertion point is because I am not sure if omp::PrivateClauseOp is supported by FirOpBuilder::getAllocaBlock. I didn't try to fix it because I don't see why we would generate IR like that. See RFC: Full diff: https://github.com/llvm/llvm-project/pull/93545.diff 4 Files Affected:
diff --git a/flang/include/flang/Optimizer/Transforms/Passes.h b/flang/include/flang/Optimizer/Transforms/Passes.h
index ebdd60630c330..151719ea71ef0 100644
--- a/flang/include/flang/Optimizer/Transforms/Passes.h
+++ b/flang/include/flang/Optimizer/Transforms/Passes.h
@@ -51,6 +51,7 @@ namespace fir {
#define GEN_PASS_DECL_STACKARRAYS
#define GEN_PASS_DECL_LOOPVERSIONING
#define GEN_PASS_DECL_ADDALIASTAGS
+#define GEN_PASS_DECL_OMPMAPINFOFINALIZATIONPASS
#include "flang/Optimizer/Transforms/Passes.h.inc"
std::unique_ptr<mlir::Pass> createAffineDemotionPass();
@@ -70,7 +71,6 @@ std::unique_ptr<mlir::Pass> createAlgebraicSimplificationPass();
std::unique_ptr<mlir::Pass>
createAlgebraicSimplificationPass(const mlir::GreedyRewriteConfig &config);
-std::unique_ptr<mlir::Pass> createOMPMapInfoFinalizationPass();
std::unique_ptr<mlir::Pass> createOMPFunctionFilteringPass();
std::unique_ptr<mlir::OperationPass<mlir::ModuleOp>>
createOMPMarkDeclareTargetPass();
diff --git a/flang/include/flang/Optimizer/Transforms/Passes.td b/flang/include/flang/Optimizer/Transforms/Passes.td
index f494da555f5ae..b0f1ad61251ce 100644
--- a/flang/include/flang/Optimizer/Transforms/Passes.td
+++ b/flang/include/flang/Optimizer/Transforms/Passes.td
@@ -324,14 +324,13 @@ def LoopVersioning : Pass<"loop-versioning", "mlir::func::FuncOp"> {
}
def OMPMapInfoFinalizationPass
- : Pass<"omp-map-info-finalization", "mlir::func::FuncOp"> {
+ : Pass<"omp-map-info-finalization"> {
let summary = "expands OpenMP MapInfo operations containing descriptors";
let description = [{
Expands MapInfo operations containing descriptor types into multiple
MapInfo's for each pointer element in the descriptor that requires
explicit individual mapping by the OpenMP runtime.
}];
- let constructor = "::fir::createOMPMapInfoFinalizationPass()";
let dependentDialects = ["mlir::omp::OpenMPDialect"];
}
diff --git a/flang/include/flang/Tools/CLOptions.inc b/flang/include/flang/Tools/CLOptions.inc
index 61ea7a7f9bbdd..e0141a3d76f0a 100644
--- a/flang/include/flang/Tools/CLOptions.inc
+++ b/flang/include/flang/Tools/CLOptions.inc
@@ -345,7 +345,8 @@ inline void createHLFIRToFIRPassPipeline(
/// rather than the host device.
inline void createOpenMPFIRPassPipeline(
mlir::PassManager &pm, bool isTargetDevice) {
- pm.addPass(fir::createOMPMapInfoFinalizationPass());
+ addNestedPassToAllTopLevelOperations(
+ pm, fir::createOMPMapInfoFinalizationPass);
pm.addPass(fir::createOMPMarkDeclareTargetPass());
if (isTargetDevice)
pm.addPass(fir::createOMPFunctionFilteringPass());
diff --git a/flang/lib/Optimizer/Transforms/OMPMapInfoFinalization.cpp b/flang/lib/Optimizer/Transforms/OMPMapInfoFinalization.cpp
index 5a5d4b9e0da4e..35203fe89f5bc 100644
--- a/flang/lib/Optimizer/Transforms/OMPMapInfoFinalization.cpp
+++ b/flang/lib/Optimizer/Transforms/OMPMapInfoFinalization.cpp
@@ -76,7 +76,9 @@ class OMPMapInfoFinalizationPass
// alloca.
if (mlir::isa<fir::BaseBoxType>(descriptor.getType())) {
mlir::OpBuilder::InsertPoint insPt = builder.saveInsertionPoint();
- builder.setInsertionPointToStart(builder.getAllocaBlock());
+ mlir::Block *allocaBlock = builder.getAllocaBlock();
+ assert(allocaBlock && "No alloca block found for this top level op");
+ builder.setInsertionPointToStart(allocaBlock);
auto alloca = builder.create<fir::AllocaOp>(loc, descriptor.getType());
builder.restoreInsertionPoint(insPt);
builder.create<fir::StoreOp>(loc, descriptor, alloca);
@@ -217,17 +219,23 @@ class OMPMapInfoFinalizationPass
mapClauseOwner.getMapOperandsMutable().assign(newMapOps);
}
- // This pass executes on mlir::ModuleOp's finding omp::MapInfoOp's containing
- // descriptor based types (allocatables, pointers, assumed shape etc.) and
- // expanding them into multiple omp::MapInfoOp's for each pointer member
- // contained within the descriptor.
+ // This pass executes on omp::MapInfoOp's containing descriptor based types
+ // (allocatables, pointers, assumed shape etc.) and expanding them into
+ // multiple omp::MapInfoOp's for each pointer member contained within the
+ // descriptor.
+ //
+ // From the perspective of the MLIR pass manager this runs on the top level
+ // operation (usually function) containing the MapInfoOp because this pass
+ // will mutate siblings of MapInfoOp.
void runOnOperation() override {
- mlir::func::FuncOp func = getOperation();
- mlir::ModuleOp module = func->getParentOfType<mlir::ModuleOp>();
+ mlir::ModuleOp module =
+ mlir::dyn_cast_or_null<mlir::ModuleOp>(getOperation());
+ if (!module)
+ module = getOperation()->getParentOfType<mlir::ModuleOp>();
fir::KindMapping kindMap = fir::getKindMapping(module);
fir::FirOpBuilder builder{module, std::move(kindMap)};
- func->walk([&](mlir::omp::MapInfoOp op) {
+ getOperation()->walk([&](mlir::omp::MapInfoOp op) {
// TODO: Currently only supports a single user for the MapInfoOp, this
// is fine for the moment as the Fortran Frontend will generate a
// new MapInfoOp per Target operation for the moment. However, when/if
@@ -253,9 +261,3 @@ class OMPMapInfoFinalizationPass
};
} // namespace
-
-namespace fir {
-std::unique_ptr<mlir::Pass> createOMPMapInfoFinalizationPass() {
- return std::make_unique<OMPMapInfoFinalizationPass>();
-}
-} // namespace fir
|
There was a problem hiding this comment.
I don't have any issues with the change from what I can see, if it breaks any of the existing runtime or lit tests we will likely know about it soon enough! Although, from what I can see/understand I doubt that'd be the case :-)
However, one question I may have is, will this result in walks on the same MapInfoOp more than once? Might be a dumb question, but I am unfamiliar with addNestedPassToAllTopLevelOperations unfortunately.
The pass sadly isn't really setup to be executed multiple times on the same MapInfoOp, so if it triggers on a ModuleOp walks the MapInfoOps, and then triggers on the FuncOp functions contained within that ModuleOp, there may be an issue. As it'll expand certain (any containing BoxTypes) MapInfoOp multiple times, which will result in multiple maps of it, which in some cases will work fine, others not so much.
Otherwise, I have no issues with this PR of course and it LGTM, outside of that question! And I am more than happy to hit approve after the above question has been answered!
|
Thanks for taking a look!
It means it will run on other top level operations (currently only There's a good argument that this is a waste of time because we won't generate map ops inside of these operations. But I wanted the pass pipeline to be consistent and future proof. I expect that in practice the overhead shouldn't be too bad because only a minority of operations will be outside of |
No problem, always happy to, sometimes I miss things in the flood of emails from LLVM so feel free to ping me if I don't review or answer something quickly enough!
Thank you very much, then I don't think it should have any affect and will work like it did previously, as it was working on FuncOp before hand and just walking them for MapInfoOp's.
We will for FuncOp's but the rest not so much, still good to align with the rest I think. The only downside I would imagine is it is a lot harder to swap this to a ModuleOp for testing (print/cout testing at least) and debugging, as I imagine it'd run in parallel still like before, just a lot harder to swap to a ModuleOp. But not a big issue and maybe (probably is) there's a way to make sure an execution only utilises a single thread regardless of the operation it's executed on! Just something I am keenly aware of as this pass is still very much a WIP in a lot of areas :-) But regardless, happy to sign off on this as the question was answered and it seems to me it will work like before, at least for the moment with the current listing of top level operations. |
In this case it does walk the whole module (instead of going through If you want to disable multithreading, use |
Thank you very much for the awesome explanation, that was incredibly informative and I'll be sure to use that going forward :-) there's a lot of command line options and things I still need to learn in general, so that is very helpful. |
This is generally just for consistency with the rest of the pipeline.
The assertion for the insertion point is because I am not sure if omp::PrivateClauseOp is supported by FirOpBuilder::getAllocaBlock. I didn't try to fix it because I don't see why we would generate IR like that.
See RFC:
https://discourse.llvm.org/t/rfc-add-an-interface-for-top-level-container-operations