[PATCH] D136103: OpenMP asynchronous memory copy support

[Shilei Tian via Phabricator via cfe-commits]

tianshilei1992 requested changes to this revision.
tianshilei1992 added inline comments.
This revision now requires changes to proceed.


================
Comment at: openmp/libomptarget/src/api.cpp:208
+
+  TargetMemcpyArgsTy *Args = (TargetMemcpyArgsTy *)Task->shareds;
+
----------------
I didn't see `Args` is freed after use. There is memory leak.


================
Comment at: openmp/libomptarget/src/api.cpp:253
+  // Create task object
+  TargetMemcpyArgsTy *Args_ =
+      new TargetMemcpyArgsTy(Dst, Src, Length, DstOffset, SrcOffset, DstDevice,
----------------
Why does it need a `_` at the end?


================
Comment at: openmp/libomptarget/src/api.cpp:364
+  if (Dst == nullptr || Src == nullptr)
+    return OFFLOAD_FAIL;
+
----------------
I'm not sure if it is a failure if source and destination are same.


================
Comment at: openmp/libomptarget/src/private.h:171
+
+void __kmpc_proxy_task_completed_ooo (kmp_task_t *ptask) __attribute__((weak));
+kmp_int32 __kmpc_omp_task_with_deps (ident_t *loc_ref, kmp_int32 gtid, kmp_task_t * new_task,
----------------
proxy task is not needed in this patch. we don't have detached task.


================
Comment at: openmp/libomptarget/src/private.h:187
+      for (int i = 0; i < Depobj_count; i ++) {
+        omp_depend_t depobj = Depobj_list[i];
+        Depobjs[i] = * ((kmp_depend_info_t* )depobj);
----------------
argument name is not in LLVM style


================
Comment at: openmp/libomptarget/src/private.h:212
+public:
+  TargetMemcpyRectArgsTy(void *Dst_, const void *Src_, size_t ElementSize_, int NumDims_,
+                          const size_t* Volume_, const size_t* DstOffsets_, const size_t* SrcOffsets_,
----------------
This is really not LLVM code style.


================
Comment at: openmp/libomptarget/src/private.h:113
+
+typedef struct kmp_tasking_flags { /* Total struct must be exactly 32 bits */
+  /* Compiler flags */ /* Total compiler flags must be 16 bits */
----------------
josemonsalve2 wrote:
> This would be the third location where this struct is duplicated: interop.h, kmp.h and this file. Would it make sense to try to add it to another common header file?
IIRC, there are some (non-technical) issues on using `kmp.h` in `libomptarget`.


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

https://reviews.llvm.org/D136103