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

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert added a comment.

Only minor nits.



================
Comment at: openmp/libomptarget/src/omptarget.cpp:841
+        AlignedSize(Size + Size % Alignment) {}
+};
+
----------------
When I read `FP` I think floating point. Maybe rename all FPs into FirstPriv or similar, characters are free, confusion is costly.


================
Comment at: openmp/libomptarget/src/omptarget.cpp:974
+        FPArgSize += PrivateArgInfo.back().AlignedSize;
       }
     } else {
----------------
when you talk about "the threshold" in the comments above it is not clear what you mean. Add a (see ...) or additional words, e.g., firstprivate bundling threshold.


================
Comment at: openmp/libomptarget/src/omptarget.cpp:1005
+      Itr = std::next(Itr, Info.AlignedSize);
+    }
+    // Allocate target memory
----------------
Make HstPtr a char* in the struct. Then you don't need to unpack hstptr nor size here.


================
Comment at: openmp/libomptarget/src/omptarget.cpp:1019
   std::vector<ptrdiff_t> TgtOffsets;
-  std::vector<void *> FPArrays;
 
----------------
Why? Still multiple.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86307