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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
