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

George Rokos via Phabricator via Openmp-commits openmp-commits at lists.llvm.org
Tue Sep 1 07:40:36 PDT 2020

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

  rG LLVM Github Monorepo



More information about the Openmp-commits mailing list