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

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert added inline comments.


================
Comment at: openmp/libomptarget/plugins/cuda/src/rtl.cpp:118
+      DP("Error when setting current CUDA context\n");
+      CUDA_ERR_STRING(err);
+      // We will return if cannot switch to the right context in case of
----------------
tianshilei1992 wrote:
> jdoerfert wrote:
> > Outline this into a helper. Also the error checking should be a helper.
> > 
> > So something like:
> > ```
> > /// ....
> > bool checkResult(CUResult err, const char *ErrMsg) {
> >   if (err == CUDA_SUCCESS)
> >     return true;
> >    DP(ErrMsg);
> >    CUDA_ERR_STRING(err);
> >    return false;
> > }
> > 
> > /// ...
> > bool switchToDevice(DeviceId) {
> >   CUresult err = cuCtxSetCurrent((*ContextsPtr)[DeviceId]);
> >   return checkResult(err, "Error when creating CUDA stream to resize stream pool\n");
> > }
> > ```
> > 
> > and then use it eveywhere. Should cut down the size and duplication.
> We can do the first one. As for the second function, it must be the member function. Let's do that in next patch which moves every CUDA related things into class.
ok


================
Comment at: openmp/libomptarget/plugins/cuda/src/rtl.cpp:540
       CUDA_ERR_STRING(err);
     }
   }
----------------
tianshilei1992 wrote:
> 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`?
Once could move both to the `StreamManager::initializeDevice`, set ctx and create streams. One could then either reset the context or just document that the context is set by `initializeDevice`. This is a one time cost so setting the context twice is not my main concern.


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

https://reviews.llvm.org/D77412