[Openmp-commits] [PATCH] D74145: [OpenMP][Offloading] Added support for multiple streams so that multiple kernels can be executed concurrently

[Shilei Tian via Phabricator via Openmp-commits]

tianshilei1992 added inline comments.


================
Comment at: openmp/libomptarget/plugins/cuda/src/rtl.cpp:182
+  // Get the next stream on a given device in a round robin manner
+  CUstream &getNextStream(const int DeviceId) {
+    assert(DeviceId < static_cast<int>(NextStreamId.size()) &&
----------------
jdoerfert wrote:
> grokos wrote:
> > jdoerfert wrote:
> > > tianshilei1992 wrote:
> > > > JonChesterfield wrote:
> > > > > It looks like DeviceID should be unsigned here
> > > > Well, yes, it should be. But if you take a look at what they're used, for example at line 725, you can see the declaration is `int32_t device_id`.
> > > we make it an unsigned here. I can do that before I commit as well.
> > Well, strictly speaking, device IDs in libomptarget are signed. E.g. the default device has an ID of -1 and the host device has ID -10. On the other hand, such negative values should never reach the plugin, if that ever happens then something is buggy in the base library. So it's really up to you to either keep the signed flavor or switch to unsigned.
> signed + assertion(id >= 0) ?
It would be better we put this check in each API call.


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

https://reviews.llvm.org/D74145