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

[George Rokos via Phabricator via Openmp-commits]

grokos added a comment.

Minor comments (about typos) to be taken into account in case of a future patch.



================
Comment at: openmp/libomptarget/src/omptarget.cpp:831
+  struct FirstPrivateArgInfoTy {
+    /// The index of the element in \p TgtArgs corresponding to the argument
+    const int Index;
----------------
Here you are referring to `TgtArgs` which is an argument of `packAndTransfer()`. Can you add a comment in parenthesis to make it clear? The first time I read the code I thought you were mistakenly using the name `TgtArgs` instead of `TgtPtrs` (which is a member of `PrivateArgumentManagerTy` class).


================
Comment at: openmp/libomptarget/src/omptarget.cpp:907
+      // FirstPrivateArgSizeThreshold);
+      // 2. It must be first-private (needs to be mapped to target device).
+      // We will pack all this kind of arguments to transfer them all at once
----------------
needs to mapped --> needs to be copied to target device
Technically speaking, first-private variables are not mapped, they are only copied to the device.


================
Comment at: openmp/libomptarget/src/omptarget.cpp:928
+      assert(FirstPrivateArgSize != 0 &&
+             "FirstPrivateArgSize is 0 but FirstPrivateArgInfo is empty");
+      FirstPrivateArgBuffer.resize(FirstPrivateArgSize, 0);
----------------
is empty --> is not empty


================
Comment at: openmp/libomptarget/src/omptarget.cpp:957
+        void *&Ptr = TgtArgs[Info.Index];
+        assert(Ptr == nullptr && "Target pointer is already set by mistaken");
+        Ptr = reinterpret_cast<void *>(TP);
----------------
by mistaken --> by mistake


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86307