Conversation
Use explicit cuda_event implementation everywhere.
There was a problem hiding this comment.
Pull request overview
This PR unifies CUDA event usage by reusing the explicit cuda_event RAII wrapper (include/ghex/device/cuda/event.hpp) inside the CUDA future implementation, and aligns operator bool() semantics for CUDA stream/event with PR #199.
Changes:
- Invert
operator bool()forghex::device::streamandghex::device::cuda_eventto returntruewhen the object is valid (not moved-from). - Replace
future’s internalcudaEvent_tmanaged-struct wrapper with the sharedcuda_eventRAII type. - Update
cuda_eventto accept event creation flags and add implicit conversions tocudaEvent_treferences; adjustevent_poolassertion accordingly.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| include/ghex/device/cuda/stream.hpp | Aligns operator bool() validity semantics with the new convention. |
| include/ghex/device/cuda/future.hpp | Switches future’s event storage to cuda_event (but introduces an include-guard issue). |
| include/ghex/device/cuda/event_pool.hpp | Updates assertion to match the inverted cuda_event::operator bool() semantics. |
| include/ghex/device/cuda/event.hpp | Adds flag-configurable ctor and aligns operator bool() validity semantics; adds implicit cudaEvent_t conversions. |
Comments suppressed due to low confidence (1)
include/ghex/device/cuda/future.hpp:17
future.hppnow includesghex/device/cuda/event.hppunconditionally. That header depends on CUDA runtime types/macros and is not guarded byGHEX_CUDACC, so this will break any translation unit that includesfuture.hppwithoutGHEX_CUDACC(e.g., generic headers likeinclude/ghex/packer.hppinclude this file unconditionally). Consider moving the CUDA-only includes (device/cuda/event.hpp, and any other CUDA-runtime-dependent headers) inside the#ifdef GHEX_CUDACCblock, or include the wrapper headerghex/device/event.hppinstead so non-CUDA builds see the stub type.
#include <ghex/config.hpp>
#include <ghex/device/cuda/event.hpp>
#include <ghex/device/cuda/error.hpp>
#ifdef GHEX_CUDACC
#include <ghex/device/cuda/runtime.hpp>
#endif
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
philip-paul-mueller
left a comment
philip-paul-mueller
left a comment
There was a problem hiding this comment.
I have some comments/questions.
I kind of think that the number of places where we have amended the meaning is a bit low?
@philip-paul-mueller do you mean inverting the bool operator meaning? I think it's because the Or do you mean something else? |
This is on top of #199.
Uses the explicit implementation of a cudaEvent_t RAII wrapper in include/ghex.device/cuda/event.hpp in the device future implementation.