[Openmp-commits] [PATCH] D68100: [OpenMP 5.0] declare mapper runtime implementation

[Alexey Bataev via Phabricator via Openmp-commits]

ABataev added a comment.

In D68100#1686416 <https://reviews.llvm.org/D68100#1686416>, @lildmh wrote:

> In D68100#1686413 <https://reviews.llvm.org/D68100#1686413>, @lildmh wrote:
>
> > In D68100#1686397 <https://reviews.llvm.org/D68100#1686397>, @ABataev wrote:
> >
> > > In D68100#1686395 <https://reviews.llvm.org/D68100#1686395>, @lildmh wrote:
> > >
> > > > In D68100#1686337 <https://reviews.llvm.org/D68100#1686337>, @grokos wrote:
> > > >
> > > > > In D68100#1686044 <https://reviews.llvm.org/D68100#1686044>, @ABataev wrote:
> > > > >
> > > > > > In D68100#1685975 <https://reviews.llvm.org/D68100#1685975>, @lildmh wrote:
> > > > > >
> > > > > > > Please review when you have time, thanks
> > > > > >
> > > > > >
> > > > > > Maybe, it is better to add a single function to pass the list of mappers to the runtime and use the old functions rather than just duplicate them? Something like `__tgt_mappers(...)` and store them in the runtime. Plus, modify the original functions to use mappers if they were provided. Thoughts?
> > > > >
> > > > >
> > > > > I'm in favor of this solution, as it is less intrusive than the one posted in this patch. I think a revised patch will be quite smaller and the changes it introduces will be clearer.
> > > >
> > > >
> > > > I think Alexey and George's concern is that this patch introduces many new runtime apis. My arguments are:
> > > >
> > > > 1. This patch will deprecate the old interfaces as we discussed before in the meeting. E.g., `__tgt_target_teams` will no longer be used. They are kept there just for legacy code support. So I didn't actually introduce any new runtime apis.
> > > > 2. If we have something like `__tgt_mapper` instead, and integrate all mapper functionalities in it, we will need to pass extra arguments to it to distinguish whether it is for `__tgt_targt`, `__tgt_target_data_begin`, etc. On the other hand, the current runtime has an interface for each case. For instance, we have `__tgt_targt`, `__tgt_target_data_begin`,  `__tgt_target_data_update_nowait`, etc., instead of a single `__tgt_target` function which does it all. Therefore, I think the current design fits the overall design of OpenMP runtime better: have a function for each case.
> > >
> > >
> > > Why we will need this extra argument? It just provides a list of mapper functions and stores them in the runtime before we call any `__tgt_...` function. Each particular `__tgt_...` runtime function will know what do with all these mappers if they were stored.
> >
> >
> > Okay, I think I understand your idea now. Then in this case, we will have a call to `__tgt_mapper` before every call to `__tgt_target*`, because we need to overwrite the mappers written for previous calls. I don't particularly like this idea, since this will introduce implicit dependencies between different runtime calls and a program will have twice runtime calls. But if most people like it, I'm okay.
>
>
> I think another problem is this may not work with legacy code, since they don't have calls to `__tgt_mapper`. This may be a bigger problem when legacy code and code compiled with new Clang are mixed together: a legacy call to `__tgt_target` may get a mapper which is intended for some new code.


No, there should not be problem with the legacy code. If the array of mappers is empty - use default mapping through the bitcopying.


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

https://reviews.llvm.org/D68100