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

[Alexey Bataev via Phabricator via Openmp-commits]

ABataev added inline comments.


================
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],
----------------
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.


================
Comment at: libomptarget/src/omptarget.cpp:546
+      // Map each component filled up by the mapper function.
+      for (int32_t j = 0; j < Components.size(); ++j) {
+        const int next_j = j + 1;
----------------
Usually, we use something like `(for i = 0, e = end(); i < e; ++i)` pattern.


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

https://reviews.llvm.org/D68100