Add stream support to CUDA and HIP

[upsj]

TODO:

• test this behavior, see cuDF tests for how to do it
• add stream to sparselib/blas handles
• add RAII stream handle types for CUDA and HIP

Closes #1206

[MarcelKoch]

What happens with a user provided stream, if device_reset = true in a cuda/hip executor? For the cuda docs, it seems like using the stream again is undefined behavior.

[upsj]

yes, to be honest, I think the device_reset settings exceed the scope of a single executor and should be removed in the long term. I like to think of devices like the MPI environment: They should only be initialized and finalized globally. Streams and devices should usually have a wider scope than executors, unless they are explicitly created internally.
Similarly, if you have multiple executors on a device, and one with device_reset enabled gets destroyed, all the other executors' memory gets freed as well, crashing the whole program.

[MarcelKoch]

That sounds like a reasonable approach. Perhaps we could deprecate the constructor with the device_reset in another PR.
Right now, maybe we could issue a warning if a user sets device_reset and also a custom stream, or don't offer a constructor with both device_reset and stream parameter.
Sidenote: Our cuda/hip executors count the number of active executors on a specific device, and only does the reset if there are no other active executors. So that case should be handled 'correctly', but it might ignore device_reset even if it was set by a user.

[upsj]

Ah, thanks for the reminder, that's actually slight worse IMO: This way the order of destruction matters for whether DeviceReset gets called.

[MarcelKoch]

Reviewing this is a bit hard because there are so many smaller changes. I've done some regex searches, and it seems that there are no kernel launches without the stream parameter anymore. (Unless we are not using the <<<...>>> syntax somewhere, but I don't think so.) And the proposed testing might catch the cases we missed.
Other than that, the interface changes on the executor side look good. Perhaps we could add a constructor, so that we have one constructor with the device_reset parameter, but without the stream and vice versa.

[upsj]

format!

[pratikvn]

I think there are some changes from other PRs here.

stale

[sonarcloud]

SonarCloud Quality Gate failed.   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
26 Code Smells

57.1% Coverage
20.8% Duplication

[codecov]

Codecov Report

Patch coverage: 83.07% and project coverage change: -0.43 ⚠️

Comparison is base (5ac0478) 91.52% compared to head (24a0953) 91.10%.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1236      +/-   ##
===========================================
- Coverage    91.52%   91.10%   -0.43%     
===========================================
  Files          567      567              
  Lines        48204    48268      +64     
===========================================
- Hits         44121    43973     -148     
- Misses        4083     4295     +212     

Impacted Files	Coverage Δ
include/ginkgo/core/log/profiler_hook.hpp	100.00% <ø> (ø)
core/device_hooks/cuda_hooks.cpp	38.63% <20.00%> (-3.87%)	⬇️
core/device_hooks/hip_hooks.cpp	40.47% <20.00%> (-4.27%)	⬇️
core/solver/multigrid.cpp	88.28% <50.00%> (+0.04%)	⬆️
test/utils/mpi/executor.hpp	90.00% <85.71%> (-10.00%)	⬇️
core/log/profiler_hook.cpp	73.91% <100.00%> (+2.87%)	⬆️
core/solver/ir.cpp	87.09% <100.00%> (ø)
core/test/log/profiler_hook.cpp	95.74% <100.00%> (+0.62%)	⬆️
include/ginkgo/core/base/executor.hpp	75.59% <100.00%> (ø)
test/utils/executor.hpp	100.00% <100.00%> (ø)

... and 5 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.