[Openmp-commits] [PATCH] D77412: [OpenMP] Introduce stream pool to make sure the correctness of device synchronization

[Shilei Tian via Phabricator via Openmp-commits]

tianshilei1992 added inline comments.


================
Comment at: openmp/libomptarget/plugins/cuda/src/rtl.cpp:103
+  // Pointer to per-device context
+  std::vector<CUcontext> *ContextsPtr;
+
----------------
jdoerfert wrote:
> Later we can think abut making this a vector of structs instead as we tend to access them per device anyway. We can then align the structs such that different processes interacting with different devices don't really interfere with each other.
That would be ideal.


================
Comment at: openmp/libomptarget/plugins/cuda/src/rtl.cpp:137
+  StreamManagerTy(const int NumberOfDevices, std::vector<CUcontext> *CtxPtr)
+      : NumberOfDevices(NumberOfDevices), ContextsPtr(CtxPtr) {
+    StreamPool.resize(NumberOfDevices);
----------------
jdoerfert wrote:
> If there is no reason ever to provide a nullptr as `CtxPtr` make it a reference everywhere instead.
My bad. At the very beginning I thought by some chances it will be `nullptr` but it turns out that will never happen. :-)


================
Comment at: openmp/libomptarget/plugins/cuda/src/rtl.cpp:150
+    for (std::vector<CUstream> &S : StreamPool)
+      S.resize(EnvNumInitialStreams);
+
----------------
jdoerfert wrote:
> Don't you need to create the sreams, e.g., call resizeStreamPool instead?
Here I follow the initial design which is later initialization. In the constructor, it only allocate memory but does not initialize.


================
Comment at: openmp/libomptarget/plugins/cuda/src/rtl.cpp:540
       CUDA_ERR_STRING(err);
     }
   }
----------------
jdoerfert wrote:
> I was expecting this to be part of the StreamManager. If it can only happen at this point, maybe we want a `StreamManager::initializeDevice` method or similar. We can then also avoid exposing the stream pool to the outside. 
Yes, we could do that. But what I'm thinking is, do we assume `cuCtxSetCurrent` is called before we call `StreamManager::initializeDevice`, or we will set the context again in `StreamManager::initializeDevice`?


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

https://reviews.llvm.org/D77412