[Openmp-commits] [PATCH] D60972: [OpenMP 5.0] libomptarget interface for declare mapper functions

[Lingda Li via Phabricator via Openmp-commits]

lildmh added a comment.

In D60972#1529238 <https://reviews.llvm.org/D60972#1529238>, @Hahnfeld wrote:

> In D60972#1529215 <https://reviews.llvm.org/D60972#1529215>, @lildmh wrote:
>
> > In D60972#1529151 <https://reviews.llvm.org/D60972#1529151>, @Hahnfeld wrote:
> >
> > > In D60972#1529083 <https://reviews.llvm.org/D60972#1529083>, @lildmh wrote:
> > >
> > > > In D60972#1528662 <https://reviews.llvm.org/D60972#1528662>, @Hahnfeld wrote:
> > > >
> > > > > It would be great to have such things in public...
> > > >
> > > >
> > > > Sure, there is no secret. Please see it here if you are interested: https://github.com/lingda-li/public-sharing/blob/master/mapper_runtime_design.pptx
> > >
> > >
> > > From a quick look, I'd say this does not reflect the current design: The types are named differently, have a different layout (SoA vs AoS) and there's no implementation of `__tgt_target_mapper` in this patch as @grokos mentioned.
> >
> >
> > Hi Jonas, starting from slide 8 is the current design, `__tgt_target_mapper` etc. are deprecated. It may not accurately reflect the current code but the framework should be the same.
> >
> > > Moreover, I'd question the following things:
> > > 
> > > 1. Why are we back to `__kmpc_?` naming? Most other functions specific to libomptarget are called `__tgt_?`.
> >
> > There are other functions in libomptarget starting with `__kmpc_`, for example, `__kmpc_push_target_tripcount`.
> >  My understanding is anything that does not directly call the device starting with `__kmpc_`. The IBM and Intel compiler people seem to be okay with this naming.
>
>
> Yes, this is the only function AFAICS and it has a comment "will be revised". All other functions related to mapping start with `__tgt` so unless there is good incentive, we should follow this naming convention.


Sure, I do prefer `__tgt`, as long as everyone else is okay with it.

> 
> 
>>> 2. Do you really want to construct `std::vector`s in the compiler generated code? That's bound to cause trouble with Fortran, isn't it?
>> 
>> The compiler doesn't generate code related to `std::vector`. It's only used in the runtime implementation, so it should be okay with Fortran. Again, the IBM and Intel compiler people seem to agree with it.
> 
> Maybe I don't understand where `rt_mapper_handle` comes from. According to the design slides and D59474 <https://reviews.llvm.org/D59474>, it passed as an argument to the generated `omp_mapper[...]` function, but how is the runtime system involved in its creation? Will there be additional interface functions / changes that will call this?

The idea is the runtime will create a `MapperComponentsTy` (`std::vector`) before calling the mapper function, in, for instance, `__tgt_target_data_begin` (These parts will be implemented in later patches). When the mapper function is called, the pointer of `MapperComponentsTy` is passed to it, as `void *rt_mapper_handle`. The mapper function will call `__kmpc_push_mapper_component` using this `rt_mapper_handle`, and then the runtime can put it into the `MapperComponentsTy`


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60972/new/

https://reviews.llvm.org/D60972