In D60972#1536070, @Hahnfeld wrote:

In D60972#1533013, @lildmh wrote:

@Hahnfeld Do you really think it is necessary to pass these two functions as arguments, instead of exporting them. If you do, could you explain why?

I don't say it's necessary, but rather an option. Asked differently, why do we need to export them (and decide on externally visible names) if they're only used in a function where we can pass them as arguments?

At the moment, this patch doesn't really add any value (meaning changed behavior) to the runtime. It's needed for D59474 which is already large enough on its own, but it doesn't get to any state where it can be used.

Use tgt instead of kmpc for mapper runtime function names

@Hahnfeld Do you really think it is necessary to pass these two functions as arguments, instead of exporting them. If you do, could you explain why?

Use tgt instead of kmpc

In D60972#1530490, @Hahnfeld wrote:

In D60972#1529265, @lildmh wrote:

In D60972#1529238, @Hahnfeld wrote:

In D60972#1529215, @lildmh wrote:

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, 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

That's hard to guess from the current patch and isn't in the design slides either (or I have overlooked it). Such information should be made available to reviewers such that they can assess the implementation!

In D60972#1529238, @Hahnfeld wrote:

In D60972#1529215, @lildmh wrote:

In D60972#1529151, @Hahnfeld wrote:

In D60972#1529083, @lildmh wrote:

In 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.

In D60972#1529151, @Hahnfeld wrote:

In D60972#1529083, @lildmh wrote:

In 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.

Address Alexey's comment about mapping between function and user-defined mappers

In D60972#1528662, @Hahnfeld wrote:

In D60972#1528060, @lildmh wrote:

In D60972#1528023, @grokos wrote:

I think this patch is wrong. Where are the previous additions of target_data and __tgt_target_data_mapper? Possibly you uploaded the diff between the previous revision and the current one, whereas you need to upload the diff between origin/master and your current HEAD!

Please see the attachment of Ravi sent for the biweekly meeting to see details about this new scheme. Sorry I forgot to mention

It would be great to have such things in public...

In D60972#1528023, @grokos wrote:

I think this patch is wrong. Where are the previous additions of target_data and __tgt_target_data_mapper? Possibly you uploaded the diff between the previous revision and the current one, whereas you need to upload the diff between origin/master and your current HEAD!

In D60972#1528023, @grokos wrote:

I think this patch is wrong. Where are the previous additions of target_data and __tgt_target_data_mapper? Possibly you uploaded the diff between the previous revision and the current one, whereas you need to upload the diff between origin/master and your current HEAD!

Implement the new mapper codegen scheme

Implement the new mapper scheme.

Hi Alexey,

Fix code format

Hi Alexey,

Combine 2 pointers into one.

Hi George,

Rebase

Thanks a lot!

get rid of the from clause part.

Rebase

Thanks for the catch! I also change the name from OMPMappableExprListLocTy to OMPVarListLocTy to be more accurate. We can potentially factorize other varlist clause code with this. But it will be too large for this patch.

Change parameters to const &

Further reduce the number of parameters for map, to, from, use_device, and is_device clauses. Also rebase.

Introduce a structure OMPMappableExprListSizeTy within OMPMappableExprListClause to aggregate all 4 sizes needed by such clause, and thus reduce the number of parameters when creating a map clause. This can also be used by other similar clauses. I'll have another patch to let other clauses to use this structure as well.

Sure I'll add a codegen test with mapper. Thanks!

Hi Alexey,

1. Move mapper related info to OMPMappableExprListClause, so that to and from clauses can utilize the same infrastructure as well (real support for them will be in another patch);
2. Combine the function parameters for declare mapper and declare reduction, to reduce the function parameter number of ActOnOMPVarListClause;
3. Implement ADL for mapper, and remove mapper lookups in SemaLookup.cpp;
4. Didn't combine modifier related info into a structure, because for to and from clauses will only have possible mapper modifier, not close and always;
5. Address typos that Kelvin found;
6. Rebase.

Fix part of Alexey's comments

Hi Alexey,

In D56326#1381229, @Meinersbur wrote:

@lildmh Thanks for the patch. I can commit this patch for you. The tests run fine on my machine. However, for committing on other's behalf, we were asked to make the contributor aware that the contribution will be published under a new license. Are you OK with the new license?

Thanks a lot Alexey!

Hi Alexey,

Address review comments and rebase

Hi Alexey,

Change the type of mapper clause storage from OMPClause ** to MutableArrayRef<OMPClause *>, and rebase

Hi Alexey,

Add a full context diff