[MLIR][OpenMP] Add Lowering support for OpenMP Declare Mapper directive #117046
Conversation
TIFitis
requested review from
skatrak,
kiranchandramohan,
Leporacanthicus,
agozillon,
kiranktp and
kparzysz
llvmbot
added
flang
|
@llvm/pr-subscribers-flang-fir-hlfir Author: Akash Banerjee (TIFitis) ChangesThis patch adds HLFIR/FIR lowering support for OpenMP Declare Mapper directive. Full diff: https://github.com/llvm/llvm-project/pull/117046.diff 4 Files Affected:
diff --git a/flang/lib/Lower/OpenMP/OpenMP.cpp b/flang/lib/Lower/OpenMP/OpenMP.cpp
index a2779213a1a15a..c33bf0c9ea7a08 100644
--- a/flang/lib/Lower/OpenMP/OpenMP.cpp
+++ b/flang/lib/Lower/OpenMP/OpenMP.cpp
@@ -39,6 +39,7 @@
#include "mlir/Transforms/RegionUtils.h"
#include "llvm/ADT/STLExtras.h"
#include "llvm/Frontend/OpenMP/OMPConstants.h"
+#include <string>
using namespace Fortran::lower::omp;
@@ -2701,7 +2702,39 @@ static void
genOMP(lower::AbstractConverter &converter, lower::SymMap &symTable,
semantics::SemanticsContext &semaCtx, lower::pft::Evaluation &eval,
const parser::OpenMPDeclareMapperConstruct &declareMapperConstruct) {
- TODO(converter.getCurrentLocation(), "OpenMPDeclareMapperConstruct");
+ fir::FirOpBuilder &firOpBuilder = converter.getFirOpBuilder();
+ lower::StatementContext stmtCtx;
+ const auto &spec =
+ std::get<parser::OmpDeclareMapperSpecifier>(declareMapperConstruct.t);
+ const auto &mapperName{std::get<std::optional<parser::Name>>(spec.t)};
+ const auto &varType{std::get<parser::TypeSpec>(spec.t)};
+ const auto &varName{std::get<parser::Name>(spec.t)};
+ std::stringstream mapperNameStr;
+ if (mapperName.has_value()) {
+ mapperNameStr << mapperName->ToString();
+ } else {
+ mapperNameStr << "default_"
+ << varType.declTypeSpec->derivedTypeSpec().name().ToString();
+ }
+
+ mlir::OpBuilder::InsertPoint insPt = firOpBuilder.saveInsertionPoint();
+ firOpBuilder.setInsertionPointToStart(converter.getModuleOp().getBody());
+ auto mlirType = converter.genType(varType.declTypeSpec->derivedTypeSpec());
+ auto varVal = firOpBuilder.createTemporaryAlloc(
+ converter.getCurrentLocation(), mlirType, varName.ToString());
+ symTable.addSymbol(*varName.symbol, varVal);
+
+ mlir::omp::DeclareMapperOperands clauseOps;
+ const auto *clauseList{
+ parser::Unwrap<parser::OmpClauseList>(declareMapperConstruct.t)};
+ List<Clause> clauses = makeClauses(*clauseList, semaCtx);
+ ClauseProcessor cp(converter, semaCtx, clauses);
+ cp.processMap(converter.getCurrentLocation(), stmtCtx, clauseOps);
+ auto declMapperOp = firOpBuilder.create<mlir::omp::DeclareMapperOp>(
+ converter.getCurrentLocation(), mapperNameStr.str(), varVal, mlirType,
+ clauseOps.mapVars);
+ converter.getMLIRSymbolTable()->insert(declMapperOp.getOperation());
+ firOpBuilder.restoreInsertionPoint(insPt);
}
static void
diff --git a/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp b/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp
index 4575c90e34acdd..01ffb40daa4aa2 100644
--- a/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp
+++ b/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp
@@ -447,7 +447,8 @@ class MapInfoFinalizationPass
for (auto *user : mapOp->getUsers()) {
if (llvm::isa<mlir::omp::TargetOp, mlir::omp::TargetDataOp,
mlir::omp::TargetUpdateOp, mlir::omp::TargetExitDataOp,
- mlir::omp::TargetEnterDataOp>(user))
+ mlir::omp::TargetEnterDataOp, mlir::omp::DeclareMapperOp>(
+ user))
return user;
if (auto mapUser = llvm::dyn_cast<mlir::omp::MapInfoOp>(user))
diff --git a/flang/test/Lower/OpenMP/Todo/omp-declare-mapper.f90 b/flang/test/Lower/OpenMP/Todo/omp-declare-mapper.f90
index 5ae48ff7360482..13a4da5849f8c0 100644
--- a/flang/test/Lower/OpenMP/Todo/omp-declare-mapper.f90
+++ b/flang/test/Lower/OpenMP/Todo/omp-declare-mapper.f90
@@ -10,7 +10,7 @@ subroutine declare_mapper_1
type my_type
integer :: num_vals
integer, allocatable :: values(:)
- end type
+ end type
type my_type2
type (my_type) :: my_type_var
@@ -21,7 +21,7 @@ subroutine declare_mapper_1
type (my_type2) :: t
real :: x, y(nvals)
!$omp declare mapper (my_type :: var) map (var, var%values (1:var%num_vals))
-!CHECK: not yet implemented: OpenMPDeclareMapperConstruct
+!CHECK: not yet implemented: lowering symbol to HLFIR
end subroutine declare_mapper_1
@@ -31,7 +31,7 @@ subroutine declare_mapper_2
type my_type
integer :: num_vals
integer, allocatable :: values(:)
- end type
+ end type
type my_type2
type (my_type) :: my_type_var
@@ -43,5 +43,5 @@ subroutine declare_mapper_2
real :: x, y(nvals)
!$omp declare mapper (my_mapper : my_type2 :: v) map (v%arr, x, y(:)) &
!$omp& map (alloc : v%temp)
-!CHECK: not yet implemented: OpenMPDeclareMapperConstruct
+!CHECK: not yet implemented: lowering symbol to HLFIR
end subroutine declare_mapper_2
diff --git a/flang/test/Lower/OpenMP/declare-mapper.f90 b/flang/test/Lower/OpenMP/declare-mapper.f90
new file mode 100644
index 00000000000000..fd018b4fbb0e0f
--- /dev/null
+++ b/flang/test/Lower/OpenMP/declare-mapper.f90
@@ -0,0 +1,31 @@
+! This test checks lowering of OpenMP declare mapper Directive.
+
+! RUN: split-file %s %t
+! RUN: %flang_fc1 -emit-hlfir -fopenmp -fopenmp-version=50 %t/declare-mapper-1.f90 -o - | FileCheck %t/declare-mapper-1.f90
+! RUN %flang_fc1 -emit-hlfir -fopenmp -fopenmp-version=50 %t/declare-mapper-2.f90 -o - | FileCheck %t/declare-mapper-2.f90
+
+!--- declare-mapper-1.f90
+subroutine mapper
+ implicit none
+ type my_type
+ integer, pointer :: my_buffer
+ integer :: my_buffer_size
+ end type
+ !CHECK: %[[MY_VAR:.*]] = fir.alloca ![[VAR_TYPE:.*]] {bindc_name = "my_var"}
+ !CHECK: %[[MAP_INFO:.*]] = omp.map.info var_ptr(%[[MY_VAR]] : !fir.ref<![[VAR_TYPE]]>, ![[VAR_TYPE]]) map_clauses(tofrom) capture(ByRef) -> !fir.ref<![[VAR_TYPE]]> {name = "my_var"}
+ !CHECK: omp.declare_mapper @my_mapper : %[[MY_VAR]] : !fir.ref<![[VAR_TYPE]]> : ![[VAR_TYPE]] map_entries(%[[MAP_INFO]] : !fir.ref<![[VAR_TYPE]]>)
+ !$omp DECLARE MAPPER(my_mapper : my_type :: my_var) map(tofrom : my_var)
+end subroutine
+
+!--- declare-mapper-2.f90
+subroutine mapper_default
+ implicit none
+ type my_type
+ integer, pointer :: my_buffer
+ integer :: my_buffer_size
+ end type
+ !CHECK: %[[MY_VAR:.*]] = fir.alloca ![[VAR_TYPE:.*]] {bindc_name = "my_var"}
+ !CHECK: %[[MAP_INFO:.*]] = omp.map.info var_ptr(%[[MY_VAR]] : !fir.ref<![[VAR_TYPE]]>, ![[VAR_TYPE]]) map_clauses(tofrom) capture(ByRef) -> !fir.ref<![[VAR_TYPE]]> {name = "my_var"}
+ !CHECK: omp.declare_mapper @default_my_type : %[[MY_VAR]] : !fir.ref<![[VAR_TYPE]]> : ![[VAR_TYPE]] map_entries(%[[MAP_INFO]] : !fir.ref<![[VAR_TYPE]]>)
+ !$omp DECLARE MAPPER(my_type :: my_var) map(tofrom : my_var)
+end subroutine
\ No newline at end of file
|
|
@llvm/pr-subscribers-flang-openmp Author: Akash Banerjee (TIFitis) ChangesThis patch adds HLFIR/FIR lowering support for OpenMP Declare Mapper directive. Full diff: https://github.com/llvm/llvm-project/pull/117046.diff 4 Files Affected:
diff --git a/flang/lib/Lower/OpenMP/OpenMP.cpp b/flang/lib/Lower/OpenMP/OpenMP.cpp
index a2779213a1a15a..c33bf0c9ea7a08 100644
--- a/flang/lib/Lower/OpenMP/OpenMP.cpp
+++ b/flang/lib/Lower/OpenMP/OpenMP.cpp
@@ -39,6 +39,7 @@
#include "mlir/Transforms/RegionUtils.h"
#include "llvm/ADT/STLExtras.h"
#include "llvm/Frontend/OpenMP/OMPConstants.h"
+#include <string>
using namespace Fortran::lower::omp;
@@ -2701,7 +2702,39 @@ static void
genOMP(lower::AbstractConverter &converter, lower::SymMap &symTable,
semantics::SemanticsContext &semaCtx, lower::pft::Evaluation &eval,
const parser::OpenMPDeclareMapperConstruct &declareMapperConstruct) {
- TODO(converter.getCurrentLocation(), "OpenMPDeclareMapperConstruct");
+ fir::FirOpBuilder &firOpBuilder = converter.getFirOpBuilder();
+ lower::StatementContext stmtCtx;
+ const auto &spec =
+ std::get<parser::OmpDeclareMapperSpecifier>(declareMapperConstruct.t);
+ const auto &mapperName{std::get<std::optional<parser::Name>>(spec.t)};
+ const auto &varType{std::get<parser::TypeSpec>(spec.t)};
+ const auto &varName{std::get<parser::Name>(spec.t)};
+ std::stringstream mapperNameStr;
+ if (mapperName.has_value()) {
+ mapperNameStr << mapperName->ToString();
+ } else {
+ mapperNameStr << "default_"
+ << varType.declTypeSpec->derivedTypeSpec().name().ToString();
+ }
+
+ mlir::OpBuilder::InsertPoint insPt = firOpBuilder.saveInsertionPoint();
+ firOpBuilder.setInsertionPointToStart(converter.getModuleOp().getBody());
+ auto mlirType = converter.genType(varType.declTypeSpec->derivedTypeSpec());
+ auto varVal = firOpBuilder.createTemporaryAlloc(
+ converter.getCurrentLocation(), mlirType, varName.ToString());
+ symTable.addSymbol(*varName.symbol, varVal);
+
+ mlir::omp::DeclareMapperOperands clauseOps;
+ const auto *clauseList{
+ parser::Unwrap<parser::OmpClauseList>(declareMapperConstruct.t)};
+ List<Clause> clauses = makeClauses(*clauseList, semaCtx);
+ ClauseProcessor cp(converter, semaCtx, clauses);
+ cp.processMap(converter.getCurrentLocation(), stmtCtx, clauseOps);
+ auto declMapperOp = firOpBuilder.create<mlir::omp::DeclareMapperOp>(
+ converter.getCurrentLocation(), mapperNameStr.str(), varVal, mlirType,
+ clauseOps.mapVars);
+ converter.getMLIRSymbolTable()->insert(declMapperOp.getOperation());
+ firOpBuilder.restoreInsertionPoint(insPt);
}
static void
diff --git a/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp b/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp
index 4575c90e34acdd..01ffb40daa4aa2 100644
--- a/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp
+++ b/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp
@@ -447,7 +447,8 @@ class MapInfoFinalizationPass
for (auto *user : mapOp->getUsers()) {
if (llvm::isa<mlir::omp::TargetOp, mlir::omp::TargetDataOp,
mlir::omp::TargetUpdateOp, mlir::omp::TargetExitDataOp,
- mlir::omp::TargetEnterDataOp>(user))
+ mlir::omp::TargetEnterDataOp, mlir::omp::DeclareMapperOp>(
+ user))
return user;
if (auto mapUser = llvm::dyn_cast<mlir::omp::MapInfoOp>(user))
diff --git a/flang/test/Lower/OpenMP/Todo/omp-declare-mapper.f90 b/flang/test/Lower/OpenMP/Todo/omp-declare-mapper.f90
index 5ae48ff7360482..13a4da5849f8c0 100644
--- a/flang/test/Lower/OpenMP/Todo/omp-declare-mapper.f90
+++ b/flang/test/Lower/OpenMP/Todo/omp-declare-mapper.f90
@@ -10,7 +10,7 @@ subroutine declare_mapper_1
type my_type
integer :: num_vals
integer, allocatable :: values(:)
- end type
+ end type
type my_type2
type (my_type) :: my_type_var
@@ -21,7 +21,7 @@ subroutine declare_mapper_1
type (my_type2) :: t
real :: x, y(nvals)
!$omp declare mapper (my_type :: var) map (var, var%values (1:var%num_vals))
-!CHECK: not yet implemented: OpenMPDeclareMapperConstruct
+!CHECK: not yet implemented: lowering symbol to HLFIR
end subroutine declare_mapper_1
@@ -31,7 +31,7 @@ subroutine declare_mapper_2
type my_type
integer :: num_vals
integer, allocatable :: values(:)
- end type
+ end type
type my_type2
type (my_type) :: my_type_var
@@ -43,5 +43,5 @@ subroutine declare_mapper_2
real :: x, y(nvals)
!$omp declare mapper (my_mapper : my_type2 :: v) map (v%arr, x, y(:)) &
!$omp& map (alloc : v%temp)
-!CHECK: not yet implemented: OpenMPDeclareMapperConstruct
+!CHECK: not yet implemented: lowering symbol to HLFIR
end subroutine declare_mapper_2
diff --git a/flang/test/Lower/OpenMP/declare-mapper.f90 b/flang/test/Lower/OpenMP/declare-mapper.f90
new file mode 100644
index 00000000000000..fd018b4fbb0e0f
--- /dev/null
+++ b/flang/test/Lower/OpenMP/declare-mapper.f90
@@ -0,0 +1,31 @@
+! This test checks lowering of OpenMP declare mapper Directive.
+
+! RUN: split-file %s %t
+! RUN: %flang_fc1 -emit-hlfir -fopenmp -fopenmp-version=50 %t/declare-mapper-1.f90 -o - | FileCheck %t/declare-mapper-1.f90
+! RUN %flang_fc1 -emit-hlfir -fopenmp -fopenmp-version=50 %t/declare-mapper-2.f90 -o - | FileCheck %t/declare-mapper-2.f90
+
+!--- declare-mapper-1.f90
+subroutine mapper
+ implicit none
+ type my_type
+ integer, pointer :: my_buffer
+ integer :: my_buffer_size
+ end type
+ !CHECK: %[[MY_VAR:.*]] = fir.alloca ![[VAR_TYPE:.*]] {bindc_name = "my_var"}
+ !CHECK: %[[MAP_INFO:.*]] = omp.map.info var_ptr(%[[MY_VAR]] : !fir.ref<![[VAR_TYPE]]>, ![[VAR_TYPE]]) map_clauses(tofrom) capture(ByRef) -> !fir.ref<![[VAR_TYPE]]> {name = "my_var"}
+ !CHECK: omp.declare_mapper @my_mapper : %[[MY_VAR]] : !fir.ref<![[VAR_TYPE]]> : ![[VAR_TYPE]] map_entries(%[[MAP_INFO]] : !fir.ref<![[VAR_TYPE]]>)
+ !$omp DECLARE MAPPER(my_mapper : my_type :: my_var) map(tofrom : my_var)
+end subroutine
+
+!--- declare-mapper-2.f90
+subroutine mapper_default
+ implicit none
+ type my_type
+ integer, pointer :: my_buffer
+ integer :: my_buffer_size
+ end type
+ !CHECK: %[[MY_VAR:.*]] = fir.alloca ![[VAR_TYPE:.*]] {bindc_name = "my_var"}
+ !CHECK: %[[MAP_INFO:.*]] = omp.map.info var_ptr(%[[MY_VAR]] : !fir.ref<![[VAR_TYPE]]>, ![[VAR_TYPE]]) map_clauses(tofrom) capture(ByRef) -> !fir.ref<![[VAR_TYPE]]> {name = "my_var"}
+ !CHECK: omp.declare_mapper @default_my_type : %[[MY_VAR]] : !fir.ref<![[VAR_TYPE]]> : ![[VAR_TYPE]] map_entries(%[[MAP_INFO]] : !fir.ref<![[VAR_TYPE]]>)
+ !$omp DECLARE MAPPER(my_type :: my_var) map(tofrom : my_var)
+end subroutine
\ No newline at end of file
|
TIFitis
force-pushed
the
users/akash/mapper_lowering
branch
from
0de2f89 to
a9f42ac
Compare
flang/lib/Lower/OpenMP/OpenMP.cpp
Outdated
| auto varVal = firOpBuilder.createTemporaryAlloc( | ||
| converter.getCurrentLocation(), mlirType, varName.ToString()); |
There was a problem hiding this comment.
Sorry I didn't notice this before.
So far as I understand, this will create the fir.alloca and hlfir.declare at the beginning of the MLIR module, not nested in any intermediate operation.
How do you intend to lower this to LLVMIR? We would normally nest these in some kind of "function-like" wrapper operation e.g. func.func fir.global omp.private etc. I wonder if the declare mapper operation needs a nested region for this allocation (like we do for omp.private and omp.declare_reduction).
There was a problem hiding this comment.
From what I understood when trying to implement this lowering:
The declMapperOp's parentOp must have a symbol table as it declares a new symbol, this is why I hoisted it to the ModuleOp from the funcOp.
The declMapperOp also relies on a var definition which is immediately used in it's map clause. I can't put this var inside a region, as then it can't be used in the map clause. As such, this new alloc is also moved to just before the declMapperOp.
Do you have any suggestion to do this in a different way?
There was a problem hiding this comment.
Unfortunately I am not very familiar with the target offload parts of OpenMP so I might have missunderstood something here.
OpenMP 5.2 (section 5.8.8) says
The visibility and accessibility of this declaration are the same as those of a variable declared at the same location in the program.
Therefore I don't think it is right to be pushing this to the module scope in case there are conflicting mappers declared in different scopes in the same module.
If I understand correctly, the declare mapper directive declares var:
The variable declared by var is available for use in all map clauses on the directive, and no part of the variable to be mapped is mapped by default.
So it should be treated as though this parse node is a variable declaration so createTemporaryAlloc would be appropriate if this is inside of a function, but if it is not, this should probably be a fir.global (like if you declared a module- or file-level variable).
There was a problem hiding this comment.
Therefore I don't think it is right to be pushing this to the module scope in case there are conflicting mappers declared in different scopes in the same module.
It isn't my intention to move the declMapperOp to the module scope, but rather MLIR is forcing me to. If there's an alternative I'm missing where I can keep the declMapperOp within the func/block where it was declared, I'd much rather do that. In case there is no way to do that in MLIR, I can prepend the name of the function to the module scope declaration to prevent conflicts.
So it should be treated as though this parse node is a variable declaration so
createTemporaryAllocwould be appropriate if this is inside of a function, but if it is not, this should probably be afir.global(like if you declared a module- or file-level variable).
If we go forward with keeping the declMapperOp in the module scope, then I'll alter the var declaration to be a fir.global.
There was a problem hiding this comment.
Isn't declare mapper providing a specification which is to be used for mapping in the associated regions? The variable in the declare mapper has its scope inside the construct since it is only used for specifying how to map. Similar to what @tblah is saying, you could do something like the following.
Assuming %x is the variable that is being mapped by the rules of the declare mapper:
omp.declare_mapper @default_my_type {
^bb0(%[[VAR:.*]]: [[TYPE]]):
%v1 = omp.bounds
%v2 = omp.map.info var_ptr(%[[VAR:.*]])
omp.yield %v2
}
%x_map = omp.map.info var_ptr(%x) decl_map(@default_my_type)
omp.target_enter_data map_entries(%x_map)
or
omp.declare_mapper @default_my_type {
^bb0(%[[VAR:.*]]: [[TYPE]]):
%v1 = omp.bounds
%v2 = omp.map.info var_ptr(%[[VAR:.*]])
omp.yield %v2
}
omp.target_enter_data map_entries(%x : @default_my_type)
The alternative would be to not have a declare mapper construct and apply the declare mapper during lowering itself. But this would be early lowering and we wont be able to share code with Clang as well.
There was a problem hiding this comment.
1. If a DECLARE MAPPER directive is not specified for a type DT, a predefined mapper exists for type DT as if the type DT had appeared in the directive as follows: !$OMP DECLARE MAPPER (DT :: var) MAP (TOFROM: var) 2. If a variable is not a scalar then it is treated as if it had appeared in a map clause with a map-type of tofrom. Which is effectively equivalent to the following and extending declare mapper for non-derived types: !$OMP DECLARE MAPPER (T :: var) MAP (TOFROM: var)
I think the keyword here is likely "as if", so as long as the effects are as described it's reasonable would be my (albeit terrible) reading, and if we really wanted to be exact about the wording we'd generate/embed our own equivalent pragmas to the above for all default mappings, and then lower them, so not just at the MLIR level. However, saying that I am not against defining a default declare mapper for all cases once it's in place, it might tidy things up a bit, but it may also be more complicated/trouble than it's worth, in either case I am fine with the approach of defining default declare mappers if we'd like to go down that route :-)
I'd also love it if whatever implementation we landed on was compatible with the OpenACC implementations documentation/approach to mapping descriptors via runtime calls, as I'd like to move towards that eventually when I have some time to dig into it and see if it's viable for us. I imagine it will be, I just don't know a ton about the region'd approach so hope it wouldn't be prohibitive of this.
There was a problem hiding this comment.
I have made the required changes to both PRs to update the representation for the DeclMapperOp.
Some changes I've incorporated are:
Inside DeclMapperOp's region I've introduce a new DeclMapperInfoOp to which I've attached the MapClause. Not having any MapClause explicitly associated seemed weird to me, also walking through the region collecting all the MapInfoOps for processing in various places in the code base seemed like a bad design to me.
Also, instead of using the block arg, I've created a temporary alloca inside the region. This is to save the hassle of binding the block_arg to the symbol, binding the alloca is much more straightforward.
We can drop the entire DeclMapperOp including the region once it reaches OpenMPTOLLVMIRTranslation.
Let me what are your thoughts on this approach :)
There was a problem hiding this comment.
@kiranchandramohan @tblah @skatrak @agozillon Any thoughts on this approach?
There was a problem hiding this comment.
You should feel free to proceed with the approval of @agozillon and @skatrak. You can set up a quick call with them and describe your approach and come to a conclusion. You can report the outcomes somewhere in this patch or write in discourse.
Inside DeclMapperOp's region I've introduce a new DeclMapperInfoOp to which I've attached the MapClause. Not having any MapClause explicitly associated seemed weird to me, also walking through the region collecting all the MapInfoOps for processing in various places in the code base seemed like a bad design to me.
The idea would generally be to inline the whole declare mapper operation region, replacing the block_arg with the real variable that is going to be mapped. Would an alloca always be created for all kinds of mappings? If not committing to an alloca might mean you have to delete it in some circumstances.
We can drop the entire DeclMapperOp including the region once it reaches OpenMPTOLLVMIRTranslation.
As in just drop it without using it? Or using it create the @.omp_mapper._ZTS1T.deep_copy function for the declare mapper (#pragma omp declare mapper(deep_copy : T abc) map(abc, abc.ptr[ : abc.buf_size])) in the C example you gave below.
You have not talked about the following two points.
-> where we create the map_entries for the relevant operations (like target) for which the declare mapper implicitly applies. Currently, this is done during lowering. But in this patch you have solely focused on creating the declare mapper.
-> where the composition of map-types (map-type decay) from the map clause of declare mapper and the map clause of the relevant operation (like target) happens
Couple of other things that came to my mind :
-> Since declare mappers are in the specification section, it can also occur in modules. We have not added code to propagate it to the module file. If this is not urgent for you, I can fix it sometime.
-> You should test the case where the declare mapper is in the host subroutine
subroutine A
type t
end type
declare mapper(t)
contains
subroutine B
!$omp target
! use a var of type t
!$omp end target
end subroutine
There was a problem hiding this comment.
You should feel free to proceed with the approval of @agozillon and @skatrak.
Thanks for the go ahead, I'll consult with them before proceeding, and post any relevant findings here.
Inside DeclMapperOp's region I've introduce a new DeclMapperInfoOp to which I've attached the MapClause. Not having any MapClause explicitly associated seemed weird to me, also walking through the region collecting all the MapInfoOps for processing in various places in the code base seemed like a bad design to me.
The idea would generally be to inline the whole declare mapper operation region, replacing the block_arg with the real variable that is going to be mapped. Would an alloca always be created for all kinds of mappings? If not committing to an alloca might mean you have to delete it in some circumstances.
In case of DeclareMapper the mapping is handled in a ad-hoc basis. There is no real variable to be replaced later. The variable simply exists as a dummy to represent the mapping information for other real variables where the mapping would be used. However, this mapping isn't applied directly to those variables either, rather the runtime is notified that a mapper function exists for these variables.
We can drop the entire DeclMapperOp including the region once it reaches OpenMPTOLLVMIRTranslation.
As in just drop it without using it? Or using it create the
@.omp_mapper._ZTS1T.deep_copyfunction for the declare mapper (#pragma omp declare mapper(deep_copy : T abc) map(abc, abc.ptr[ : abc.buf_size])) in the C example you gave below.
Yes, I mean't each declareMapperOp would be resolved to a function like@.omp_mapper._ZTS1T.deep_copy. By dropping it, I mean't it's region won't be separately lowered.
You have not talked about the following two points.
-> where we create the map_entries for the relevant operations (like target) for which the declare mapper implicitly applies. Currently, this is done during lowering. But in this patch you have solely focused on creating the declare mapper. -> where the composition of map-types (map-type decay) from the map clause of declare mapper and the map clause of the relevant operation (like target) happens
I'll address the implicit mapping either in the DeclMapper lowering support for the mapClause or a separate PR soon after that.
The composition of mapClause AFAIK is likely handled in the OMPIRBuilder. If that's not the case, I'll address it in a future patch.
Couple of other things that came to my mind : -> Since declare mappers are in the specification section, it can also occur in modules. We have not added code to propagate it to the module file. If this is not urgent for you, I can fix it sometime.
Please feel free to do this if you're already familiar with what needs to be done, as I'm not. Otherwise, I can add it to my TODO list :)
-> You should test the case where the declare mapper is in the host subroutine
subroutine A type t end type declare mapper(t) contains subroutine B !$omp target ! use a var of type t !$omp end target end subroutine
Thanks, I'll test this in the mapClause mapper support PR once I have it ready.
Many thanks for the review.
TIFitis
force-pushed
the
users/akash/mapper_lowering
branch
from
a9f42ac to
17ce48e
Compare
|
As @agozillon requested, here's a sample of how Clang lowers declare mapper. C Code: LLVM IR: |
|
@kiranchandramohan I discussed the current approach with @skatrak today. When trying to implement the mapper lowering for the map clause, it became apparent that we need to add the I am however struggling with name mangling the Take the following example: Here after mangling the, Do you know any mechanism in Fortran lowering that could help resolve this issue? |
The frontend symbols also are implementing a symbol table. For the example you showed, I am assuming the symbol for Are the symbols not being resolved properly? Or did I miss a point? |
|
After the discussion with @TIFitis yesterday, I think the overall approach currently proposed makes sense. I would just like to share a few related thoughts not for this PR stack, but rather some longer term potential improvements that might make sense to do:
|
I'm working on something different at the moment, but for debugging purposes I changed the tree dumper to show symbol information too. Here's the output for this code. It shows that the variable |
|
@kiranchandramohan @kparzysz I guess I must be using the name mangler in an incorrect way then. I've added the code snippets I am using when lowering and later performing a lookup. Please let me know what would be the correct way of doing this. When lowering DeclMapperOp: When trying to lookup DeclareMapper from the mapClause mapper: |
|
|
I looked at the clang code and it's not clear to me why they do it at runtime. Mappers can refer to other mappers (for sub-objects), but all mappers and all type layouts should be present in (or obtainable from) the AST[1]. In other words, clang should be able to emit all the "expanded" map clauses (i.e. after application of the mappers) at compile-time. It's possible that instead of emitting all that code inline, they put it in a runtime function to save code size. [1] The mapper definition must be visible at the time of use in a clause, so all mappers (recursively) should be visible. |
TIFitis
force-pushed
the
users/akash/mapper_op
branch
from
00c9f3d to
4b5c92e
Compare
TIFitis
force-pushed
the
users/akash/mapper_lowering
branch
from
17ce48e to
7e504c8
Compare
|
@kiranchandramohan @skatrak @agozillon @kparzysz Hi everyone! I was on vacation these last couple of weeks, apologies for the delay. I have added PR #124746 with which we have all the implementation in place for declare mappers. It would be great to resume the review process for this PR stack. Thanks :) Note: I'll add a couple of patches to support the implicit default mapping support for declare mappers. |
|
Polite request for review 🙂 |
This patch adds the OMP.DeclareMapperOp to MLIR. The HLFIR/FIR lowering for Declare Mapper is available here #117046.
This patch adds HLFIR/FIR lowering support for OpenMP Declare Mapper directive.
Add a new name mangling method which accepts a user supplied scope to mangle the name, in addition to the existing function which always used the currentScope.
…MapperOps as well as inside the Module.
TIFitis
force-pushed
the
users/akash/mapper_lowering
branch
from
aa7f4aa to
afb17c9
Compare
This patch adds the mapper field to the omp.map.info op. Depends on #117046.
This patch adds the OMP.DeclareMapperOp to MLIR. The HLFIR/FIR lowering for Declare Mapper is available here llvm#117046.
…ve (llvm#117046) This patch adds HLFIR/FIR lowering support for OpenMP Declare Mapper directive. Depends on llvm#117045.
This patch adds the mapper field to the omp.map.info op. Depends on llvm#117046.
This patch adds the mapper field to the omp.map.info op. Depends on llvm#117046.
This patch adds the mapper field to the omp.map.info op. Depends on llvm#117046.
This patch adds HLFIR/FIR lowering support for OpenMP Declare Mapper directive.
Depends on #117045.