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

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert 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()) &&
----------------
tianshilei1992 wrote:
> 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.
true, we add them in (a) different commit(s) though. Can you add the check to the assert you have below?
(Nit: you can also use `int(NextStreamId.size())` to save some characters)


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

https://reviews.llvm.org/D74145