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

[Lingda Li via Phabricator via Openmp-commits]

lildmh added a comment.

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.


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

https://reviews.llvm.org/D68100