[Openmp-commits] [PATCH] D136103: OpenMP asynchronous memory copy support

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert added a comment.

Thanks for the latest updates. The duplication is still my main concern here. See below.



================
Comment at: openmp/libomptarget/src/api.cpp:343
+static int __kmpc_target_memcpy_rect_async_helper(kmp_int32 Gtid,
+                                                  kmp_task_t *Task) {
+  if (Task == nullptr)
----------------
My point about the "5 character difference" was that this function is basically the same as `__kmpc_target_memcpy_async_helper` and similarly `omp_target_memcpy_rect_async` is almost the same as `omp_target_memcpy_async`. Can we use common helper functions to avoid duplicating all that code twice? 
(Put differently: I assume that you implemented one of these functions and then copied it and renamed a few things to get the other. Doing that should have made it obvious that there is duplication and we want to avoid duplication.) 


================
Comment at: openmp/libomptarget/src/api.cpp:409
+      omp_depend_t DepObj = DepObj_List[i];
+      DepObjs[i] = *((kmp_depend_info_t *)DepObj);
+    }
----------------
That's not how vectors work. `push_back(...)`


================
Comment at: openmp/libomptarget/src/private.h:222
+        DstDimensions(DstDimensions_), SrcDimensions(SrcDimensions_),
+        DstDevice(DstDevice_), SrcDevice(SrcDevice_){};
+};
----------------
These structs need doxygen comments describing where they are used.


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

https://reviews.llvm.org/D136103