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

[Jon Chesterfield via Phabricator via Openmp-commits]

JonChesterfield added a comment.

In D68100#1759099 <https://reviews.llvm.org/D68100#1759099>, @jdoerfert wrote:

> @ABataev @JonChesterfield ping


My thoughts are the same as before. This change mixes a refactor with a functional change plus duplicates a bunch of code. The overall change might work but I can't tell from the diff.



================
Comment at: libomptarget/src/omptarget.cpp:550
+          (void (*)(void *, void *, void *, int64_t, int64_t))(arg_mappers[i]);
+      // The mapper function fills up Components.
+      (*mapper_func_ptr)((void *)&Components, args_base[i], args[i],
----------------
ABataev wrote:
> lildmh wrote:
> > ABataev wrote:
> > > lildmh wrote:
> > > > JonChesterfield wrote:
> > > > > The rest of this looks quite familiar too. Perhaps factor the copy & paste into helper functions that are called by both locations?
> > > > The duplication is not too much though. Do you think it will worth it to have a helper function?
> > > +1 for refactoring.
> > Hi Alexey and Jon,
> > 
> > I didn't find an elegant way to merge the code below. It's mainly because they have different way to access other components:
> > E.g., for mapper, `Components.get(parent_idx)` is used to get its parent, on the other hand, `args[parent_idx]` is used for arguments. One is array of struct, the other is struct of array.
> Still, do not understand what is a problem with the refactoring. You can use lambdas, if need some differences in data, or something similar. Anyway, it would better rather than just copy-paste.
Some options:
- wrap object in a class that adapts the interface
- pass in a function that does the access
- refactor one data type to the same layout as the other
- extract small functions which are called by both


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

https://reviews.llvm.org/D68100