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

[Ye Luo via Phabricator via Openmp-commits]

ye-luo added a comment.

Down the road, we may need a way to allocate host pinned memory via the plugin for the host buffer to maximize transfer performance.



================
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
----------------
I think only an index instead of TgtArgs is needed.


================
Comment at: openmp/libomptarget/src/omptarget.cpp:924
+  /// Pack first-private arguments and start the transfer
+  int packAndTransfer(std::vector<void *> &TgtArgs) {
+    if (!FirstPrivateArgInfo.empty()) {
----------------
Document the use of TgtArgs please.


================
Comment at: openmp/libomptarget/src/omptarget.cpp:970
+  /// Free all target memory allocated for private arguments
+  int free() {
+    for (void *P : TgtPtrs) {
----------------
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.


================
Comment at: openmp/libomptarget/src/omptarget.cpp:1065
+      const bool IsFirstPrivate = ArgTypes[I] & OMP_TGT_MAPTYPE_TO;
+      const int64_t ArgSize = ArgSizes[I];
+      Ret = PrivateArgumentManager.addArg(HstPtrBegin, ArgSize, TgtBaseOffset,
----------------
Directly use ArgSizes[I] in the  next line.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86307