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