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

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert added a comment.

First set of comments.



================
Comment at: clang/docs/ReleaseNotes.rst:253
+  `Issue 47177 <https://github.com/llvm/llvm-project/issues/47177>`_
+  `Issue 47179 <https://github.com/llvm/llvm-project/issues/47179>`_
 
----------------
Unrelated, probably bad diff origin.


================
Comment at: openmp/libomptarget/src/api.cpp:204
+// The helper function that calls omp_target_memcpy 
+int __kmpc_target_memcpy_async_helper(kmp_int32 gtid, kmp_task_t *task) {
+  if (task == 0)
----------------
`static` also below.


================
Comment at: openmp/libomptarget/src/api.cpp:208
+
+  TargetMemcpyArgsTy* args = (TargetMemcpyArgsTy *)task->shareds;
+
----------------
LLVM codeing style, please. So here, `Args`


================
Comment at: openmp/libomptarget/src/api.cpp:211
+  if (args == 0)
+    return -1;
+
----------------
not 0 but nullptr, also lots of other places.


================
Comment at: openmp/libomptarget/src/api.cpp:217
+
+  return 0;
+}
----------------
Why ignore the return value of memcpy and return 0 here?


================
Comment at: openmp/libomptarget/src/api.cpp:233
+  if (Dst == 0 || Src == 0)
+    return 5;
+
----------------
5? Why 5?


================
Comment at: openmp/libomptarget/src/api.cpp:238
+  int errsz = sizeof(kmp_task_t);
+  int errhr = 0;
+  int gtid = __kmpc_global_thread_num(NULL);
----------------
Proper names, please. Also coding style.


================
Comment at: openmp/libomptarget/src/api.cpp:243
+  kmp_int32 flags = 0;
+  kmp_tasking_flags_t *input_flags = (kmp_tasking_flags_t *)&flags;
+  input_flags->hidden_helper = 1;
----------------
This is very suspicious. Why can't we have a `kmp_tasking_flags_t` object?


================
Comment at: openmp/libomptarget/src/api.cpp:256
+  // omp_target_memcpy(Dst, Src, Length, DstOffset, SrcOffset, DstDevice, SrcDevice);
+  __kmpc_omp_task_with_deps(NULL, gtid, ptr, Depobj_count, args_->Depobjs, 0, NULL);
+
----------------
Does this return anything? The use of Rc seems useless. Why do you access args_ for some parts and not for others? That said, where does the hidden helper need access to the dependences anyway?


================
Comment at: openmp/libomptarget/src/api.cpp:387
+  return Rc;
+}
+
----------------
This screams helper as it is literally the same code modulo 5 characters in a few places. Please refactor and run clang-format on the patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136103