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

[Lingda Li via Phabricator via Openmp-commits]

lildmh added inline comments.


================
Comment at: libomptarget/src/omptarget.cpp:355
+      //                                           size_t size, int64_t type);
+      void (*mapper_func_ptr)(void *, void *, void *, int64_t, int64_t);
+      mapper_func_ptr =
----------------
JonChesterfield wrote:
> typedef/using instead of copying the type list for the cast?
Sounds good, thanks


================
Comment at: libomptarget/src/omptarget.cpp:361-364
+      if (Components.size() >= 0xffff) {
+        DP("The number of components exceed the limitation\n");
         return OFFLOAD_FAIL;
       }
----------------
ABataev wrote:
> Why we have this limitation?
It's because that's the length of the parent bit. If we have more components, the parent bits will break.


================
Comment at: libomptarget/src/omptarget.cpp:438
+
+  if ((Type & OMP_TGT_MAPTYPE_MEMBER_OF) &&
+      !(Type & OMP_TGT_MAPTYPE_PTR_AND_OBJ)) {
----------------
JonChesterfield wrote:
> It's probably worth doing the arg_types[i] => Type refactor first. Combined with the whitespace change it's quite a lot of this diff.
I extracted the mapping of each component into a separate function for code reuse purpose, like `target_data_end_component` here. It uses `Type` as the input argument, so there is no longer `arg_types[i]`. It's the same for `Size`.

So I don't think it will make it more clear to change `arg_types[i]` to `Type` first. What do you think?


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


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

https://reviews.llvm.org/D68100