[PATCH] D136103: OpenMP asynchronous memory copy support

This revision was automatically updated to reflect the committed changes. Closed by commit rG4753a4e31169: [OpenMP] asynchronous memory copy support (authored by jz10, committed by jplehr). Changed prior to commit: https://reviews.llvm.org/D136103?vs=509583&id=509775#toc Repository: rG LLVM

[PATCH] D136103: OpenMP asynchronous memory copy support

jdoerfert accepted this revision. jdoerfert added a comment. LG Comment at: openmp/libomptarget/src/api.cpp:386 + // Need to check this first to not return OFFLOAD_FAIL instead + if (!(Dst || Src)) { +DP("Call to omp_target_memcpy_rect returns max supported dimensions %d\

[PATCH] D136103: OpenMP asynchronous memory copy support

jplehr updated this revision to Diff 509583. jplehr added a comment. Removed accidentally added code Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136103/new/ https://reviews.llvm.org/D136103 Files: openmp/libomptarget/include/interop.h openmp

[PATCH] D136103: OpenMP asynchronous memory copy support

jplehr updated this revision to Diff 509581. jplehr added a comment. Fix bug to corectly support the maximally supported dimensions as required by the spec. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136103/new/ https://reviews.llvm.org/D136103

[PATCH] D136103: OpenMP asynchronous memory copy support

jplehr updated this revision to Diff 509023. jplehr added a comment. Rebased and enabled tests for generic devices. Resulted in one test failure Failed Tests (3): libomptarget :: amdgcn-amd-amdhsa :: api/omp_target_memcpy_rect_async1.c libomptarget :: x86_64-pc-linux-gnu :: api/omp_target_memcpy_

[PATCH] D136103: OpenMP asynchronous memory copy support

jplehr updated this revision to Diff 508955. jplehr added a comment. Herald added a subscriber: sunshaoce. Rebase to make ready for land Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136103/new/ https://reviews.llvm.org/D136103 Files: openmp/lib

[PATCH] D136103: OpenMP asynchronous memory copy support

jdoerfert added a comment. Any reason this hasn't been pushed yet? 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-b

[PATCH] D136103: OpenMP asynchronous memory copy support

tianshilei1992 accepted this revision. tianshilei1992 added a comment. This revision is now accepted and ready to land. Thanks! LG. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136103/new/ https://reviews.llvm.org/D136103 ___ cfe-commits mai

[PATCH] D136103: OpenMP asynchronous memory copy support

jdoerfert accepted this revision. jdoerfert added a comment. LG, thanks for all the changes. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136103/new/ https://reviews.llvm.org/D136103 ___ cfe-commits mailing list cfe-commits@lists.llvm.org ht

[PATCH] D136103: OpenMP asynchronous memory copy support

jz10 updated this revision to Diff 470684. jz10 added a comment. Thanks Shilei 1. "Can you check all resolved comments to make sure there is no open comments?" checked through the comments you and Johannes made, no more issues 2. "Since this function is not part of `libomp` and it's not gonna b

[PATCH] D136103: OpenMP asynchronous memory copy support

tianshilei1992 added a comment. Generally looks good to me. Can you check all resolved comments to make sure there is no open comments? Comment at: openmp/libomptarget/src/api.cpp:206 +// The helper function that calls omp_target_memcpy or omp_target_memcpy_rect +static int __

[PATCH] D136103: OpenMP asynchronous memory copy support

jz10 added a subscriber: jdoerfert. jz10 added a comment. hi Johannes and Shilei, is there revision that needs to be done on this patch? please let me know CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136103/new/ https://reviews.llvm.org/D136103 ___

[PATCH] D136103: OpenMP asynchronous memory copy support

jz10 updated this revision to Diff 470357. jz10 added a comment. Thanks Shilei 1. "Does it work on AMDGPU and other targets? Why does it require Nvidia here?" No, I just remove this line 2. "We don't have any performance test cases yet, and I'm not sure we need them right now." Yes, I remove

[PATCH] D136103: OpenMP asynchronous memory copy support

tianshilei1992 added inline comments. Comment at: openmp/libomptarget/test/api/omp_target_memcpy_async1.c:2 +// RUN: %libomptarget-compile-run-and-check-nvptx64-nvidia-cuda +// REQUIRES: nvptx64-nvidia-cuda + Does it work on AMDGPU and other targets? Why does it

[PATCH] D136103: OpenMP asynchronous memory copy support

jz10 updated this revision to Diff 470354. jz10 added a comment. Thanks Shilei Add RUN line for each test cases CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136103/new/ https://reviews.llvm.org/D136103 Files: openmp/libomptarget/include/interop.h openmp/libomptarget/src/api.cpp

[PATCH] D136103: OpenMP asynchronous memory copy support

tianshilei1992 added inline comments. Comment at: openmp/libomptarget/test/api/omp_target_memcpy_async1.c:1 +// Test case for omp_target_memcpy_async, oringally from GCC + There is no `RUN` line here so the test will not be triggered. Can you refer to other tes

[PATCH] D136103: OpenMP asynchronous memory copy support

jz10 updated this revision to Diff 470339. jz10 added a comment. Thanks Johannes and Shilei I added few test cases for asynchronous routine at test/api folder CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136103/new/ https://reviews.llvm.org/D136103 Files: openmp/libomptarget/includ

[PATCH] D136103: OpenMP asynchronous memory copy support

tianshilei1992 added a comment. `openmp/libomptarget/test/api/` is where we usually tests those APIs. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136103/new/ https://reviews.llvm.org/D136103 ___ cfe-commits mailing list cfe-commits@lists.ll

[PATCH] D136103: OpenMP asynchronous memory copy support

jz10 added a comment. Sure, where should I add those tests? 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/mail

[PATCH] D136103: OpenMP asynchronous memory copy support

tianshilei1992 added a comment. In D136103#3880651 , @jz10 wrote: > I checked through private.h, this header actually does the functionality > that contains all kmp and kmpc related data structures and APIs, so should > we still have to split a separate h

[PATCH] D136103: OpenMP asynchronous memory copy support

jz10 added a comment. I checked through private.h, this header actually does the functionality that contains all kmp and kmpc related data structures and APIs, so should we still have to split a separate header file? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136103/new/ https://revi

[PATCH] D136103: OpenMP asynchronous memory copy support

jz10 added a comment. Regarding this, can we just move those two helper functions to private.h ? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136103/new/ https://reviews.llvm.org/D136103 ___ cfe-commits mailing list cfe-commits@lists.llvm.or

[PATCH] D136103: OpenMP asynchronous memory copy support

tianshilei1992 added a comment. > we used both kmp relevent data structure/types and APIs, so should I wrap all > those relevant code into several tool functions and put them into separate > header file? IMO we can put all KMP related code into one header and include it where needed. For the o

[PATCH] D136103: OpenMP asynchronous memory copy support

jz10 updated this revision to Diff 469845. jz10 added a comment. Thanks Johannes I revised those issues, please check if those work CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136103/new/ https://reviews.llvm.org/D136103 Files: openmp/libomptarget/include/interop.h openmp/libomp

[PATCH] D136103: OpenMP asynchronous memory copy support

jdoerfert added a comment. Looks good from my end. Thanks for the updates, I hope they make sense in the end. Some minor style comments below. @tianshilei1992 Should accept once the header concerns are resolved. Comment at: openmp/libomptarget/src/api.cpp:256-257 +// Tas

[PATCH] D136103: OpenMP asynchronous memory copy support

jz10 updated this revision to Diff 469810. jz10 added a comment. Thanks Johannes 1. 'I feel like I'm missing something. As said before, all but 3 lines are identical in these two functions. Now you created a helper for 1/3 of those identical lines but left the other 2/3 being duplicated. Could

[PATCH] D136103: OpenMP asynchronous memory copy support

jdoerfert added a comment. In D136103#3876097 , @jz10 wrote: > Thanks Johannes and Shilei > > 1. '385-387 are the same as in omp_target_memcpy_async. Can we also not > duplicate those lines?' > > I put the common code (i.e.helper task creation) into anot

[PATCH] D136103: OpenMP asynchronous memory copy support

jz10 updated this revision to Diff 469772. jz10 added a comment. Thanks Johannes and Shilei 1. '385-387 are the same as in omp_target_memcpy_async. Can we also not duplicate those lines?' I put the common code (i.e.helper task creation) into another static function 2. 'In this code there are a

[PATCH] D136103: OpenMP asynchronous memory copy support

tianshilei1992 added inline comments. Comment at: openmp/libomptarget/src/private.h:101 typedef int kmp_int32; +typedef int64_t kmp_int64; typedef intptr_t kmp_intptr_t; Can we put all KMP related code into a separate header, but of course not called `kmp.h`?

[PATCH] D136103: OpenMP asynchronous memory copy support

jdoerfert added inline comments. Comment at: openmp/libomptarget/src/api.cpp:405 + return Rc; +} + Now we have a first helper for the "async" part, we still should deduplicate the entry point code. Above, all but lines 385-387 are the same as in `omp_target_me

[PATCH] D136103: OpenMP asynchronous memory copy support

jz10 updated this revision to Diff 469719. jz10 added a comment. Thanks Johannes and Shilei 1. using a common helper function fixed 2. using push_back for SmallVector fixed 3. add doxygen comments for struct added, please check if that works CHANGES SINCE LAST ACTION https://reviews.llvm

[PATCH] D136103: OpenMP asynchronous memory copy support

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, +

[PATCH] D136103: OpenMP asynchronous memory copy support

jz10 updated this revision to Diff 469328. jz10 added a comment. redo the proper patch, i.e. get rid of content related to clang/doc CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136103/new/ https://reviews.llvm.org/D136103 Files: openmp/libomptarget/include/interop.h openmp/libomp

[PATCH] D136103: OpenMP asynchronous memory copy support

jz10 updated this revision to Diff 469112. jz10 added a comment. 1. use SmallVector fixed 2. "5 characters" ran clang-format, please check if that works CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136103/new/ https://reviews.llvm.org/D136103 Files: clang/docs/ReleaseNotes.rst

[PATCH] D136103: OpenMP asynchronous memory copy support

jz10 updated this revision to Diff 469110. jz10 added a comment. Thanks Johannes 1. use SmallVector fixed 2. "module 5 characters" ran clang-format , please check it that works CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136103/new/ https://reviews.llvm.org/D136103 Files: clang

[PATCH] D136103: OpenMP asynchronous memory copy support

jdoerfert added inline comments. Comment at: openmp/libomptarget/src/api.cpp:264 + if (DepObj_Count > 0) { +DepObjs = new kmp_depend_info_t[DepObj_Count]; +for (int i = 0; i < DepObj_Count; i++) { Use an llvm::SmallVector Comment at: o

[PATCH] D136103: OpenMP asynchronous memory copy support

jz10 updated this revision to Diff 469060. jz10 added a comment. one minors fix, i.e. delete the depobj_list from TargetMemcpyArgsTy struct CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136103/new/ https://reviews.llvm.org/D136103 Files: clang/docs/ReleaseNotes.rst openmp/libomptar

[PATCH] D136103: OpenMP asynchronous memory copy support

jz10 updated this revision to Diff 469051. jz10 added a comment. Thanks Johannes and Shilei 1. "So, you are saying the task_with_deps function does *not* copy the dependences and therefore the array has to outlive the function?" I checked the omp_task_with_deps, it does some copy operations , s

[PATCH] D136103: OpenMP asynchronous memory copy support

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

[PATCH] D136103: OpenMP asynchronous memory copy support

jz10 added a comment. I'm not sure if it copies, will check it to confirm 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.or

[PATCH] D136103: OpenMP asynchronous memory copy support

jdoerfert added a comment. > 6. 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? > > there's type cast for depend objects from 'omp_depend_t' to > 'kmp_depend_info_t*', and the array of casted depend obj

[PATCH] D136103: OpenMP asynchronous memory copy support

jz10 updated this revision to Diff 468695. jz10 added a comment. Thanks Johannes for your comments, and I relied them below 1. format issues I ran clang-format to reformat, please check if there's any missed things; 2. replace '0' with 'nullptr' fixed 3. proper return value for helper functio

[PATCH] D136103: OpenMP asynchronous memory copy support

jdoerfert added a comment. First set of comments. Comment at: clang/docs/ReleaseNotes.rst:253 + `Issue 47177 `_ + `Issue 47179 `_ Unrelated, probably bad d

[PATCH] D136103: OpenMP asynchronous memory copy support

josemonsalve2 added a comment. Thanks for implementing this. I have added a comment inlined. 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 b

[PATCH] D136103: OpenMP asynchronous memory copy support

jz10 created this revision. jz10 added a reviewer: jdoerfert. Herald added subscribers: guansong, yaxunl. Herald added a project: All. jz10 requested review of this revision. Herald added subscribers: openmp-commits, cfe-commits, sstefan1. Herald added projects: clang, OpenMP. We introduced the im