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

[Jon Chesterfield via Phabricator via Openmp-commits]

JonChesterfield added a comment.

I think the direction is good here. Some duplication is inevitable in the interface. Between the non functional changes and the code duplication in the implementation it's difficult to work out exactly what the code is doing though.



================
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 =
----------------
typedef/using instead of copying the type list for the cast?


================
Comment at: libomptarget/src/omptarget.cpp:438
+
+  if ((Type & OMP_TGT_MAPTYPE_MEMBER_OF) &&
+      !(Type & OMP_TGT_MAPTYPE_PTR_AND_OBJ)) {
----------------
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.


================
Comment at: libomptarget/src/omptarget.cpp:466
+           Size, DPxPTR(TgtPtrBegin), DPxPTR(HstPtrBegin));
+        int rt = Device.data_retrieve(HstPtrBegin, TgtPtrBegin, Size);
         if (rt != OFFLOAD_SUCCESS) {
----------------
Likewise data_size => Size. Separating the NFC from the FC makes it easier to parse the latter.


================
Comment at: libomptarget/src/omptarget.cpp:547
+      //                                           size_t size, int64_t type);
+      void (*mapper_func_ptr)(void *, void *, void *, int64_t, int64_t);
+      mapper_func_ptr =
----------------
I think this is the same type list copy & paste that suggested a typedef above


================
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],
----------------
The rest of this looks quite familiar too. Perhaps factor the copy & paste into helper functions that are called by both locations?


================
Comment at: libomptarget/src/omptarget.cpp:698
+      //                                           size_t size, int64_t type);
+      void (*mapper_func_ptr)(void *, void *, void *, int64_t, int64_t);
+      mapper_func_ptr =
----------------
And again


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

https://reviews.llvm.org/D68100