[Openmp-commits] [PATCH] D86307: [OpenMP] Pack first-private arguments to improve efficiency of data transfer

[Shilei Tian via Phabricator via Openmp-commits]

tianshilei1992 marked 4 inline comments as done.
tianshilei1992 added inline comments.


================
Comment at: openmp/libomptarget/src/omptarget.cpp:872
+  int addArg(void *HstPtr, int64_t ArgSize, int64_t ArgOffset,
+             bool IsFirstPrivate, void *&TgtPtr, std::vector<void *> TgtArgs) {
+    // If the argument is not first-private, or its size is greater than a
----------------
ye-luo wrote:
> I think only an index instead of TgtArgs is needed.
It would be not very straightforward why we need an index here, and there is no difference in terms of performance between an index and a reference.


================
Comment at: openmp/libomptarget/src/omptarget.cpp:970
+  /// Free all target memory allocated for private arguments
+  int free() {
+    for (void *P : TgtPtrs) {
----------------
ye-luo wrote:
> In my first thought, I feel better to mark this function private and called by the destructor only. Requiring free() to be called explicitly is error-prone. In a second thought, this probably needed to propagate the return error.
Yes, that's exactly the reason that we have a function here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86307