[Mirror] Introduction of Kokkos::Event interface (based on and includes 9122) - #15
[Mirror] Introduction of Kokkos::Event interface (based on and includes 9122)#15csiefer2 wants to merge 11 commits into
Conversation
Introduces Kokkos::Experimental::CudaEvent, a lightweight RAII wrapper around cudaEvent_t that enables fine-grained synchronization between CUDA streams. Includes a portable Event API with add_dependency_to member and space_depends_on free function for expressing execution space stream dependencies, along with backend-specific CudaEvent tests and portable Event test infrastructure. Signed-off-by: Jonathan Wong <131846439+nvjonwong@users.noreply.github.com>
Replace backend-specific CudaEvent with a generic Event<Cuda> template. Use shared_ptr for copyable refcounting, expose space_depends_on as Kokkos::Experimental free functions, and add a recording constructor replacing the static record_event() factory. Add cross-stream overlap timing tests using partition_space. Signed-off-by: Jonathan Wong <131846439+nvjonwong@users.noreply.github.com>
fence() better matches Kokkos' existing conventions. Co-authored-by: Christian Trott <crtrott@sandia.gov> Signed-off-by: Jonathan Wong <131846439+nvjonwong@users.noreply.github.com>
Wall-clock timing tests are inherently non-deterministic, making them unreliable in CI environments. Remove TestCuda_CudaEvent.cpp and its CMake reference. Portable correctness tests in TestEvent.hpp remain and provide sufficient coverage. Signed-off-by: Jonathan Wong <131846439+nvjonwong@users.noreply.github.com>
Move the generic Event API to Kokkos_Event.hpp and place the CUDA specialization in Cuda/Kokkos_Cuda_Event.hpp. Extract the CUDA event handle as Impl::EventHandle<Cuda> and set the CUDA device around event operations. Signed-off-by: Jonathan Wong <131846439+nvjonwong@users.noreply.github.com>
Rename the backend event handle resource and update the CUDA specialization to use the requested event/device member names. Keep CUDA device selection for event creation, remove unnecessary device switches around event sync/query/destroy and stream waits, and adjust the portable event tests for the updated behavior. Signed-off-by: Jonathan Wong <131846439+nvjonwong@users.noreply.github.com>
Apply the module-aware Kokkos include path to TestEventHelpers.hpp. Co-authored-by: Daniel Arndt <arndtd@ornl.gov> Signed-off-by: Jonathan Wong <131846439+nvjonwong@users.noreply.github.com>
Move the CUDA Event specialization include out of the generic Event header and into the CUDA declaration header. Let the CUDA Event default constructor delegate through the execution-space constructor. Inline the remaining portable Event test functors into TestEvent.hpp and remove the now-unused helper header. Signed-off-by: Jonathan Wong <131846439+nvjonwong@users.noreply.github.com>
Include Kokkos_Event.hpp directly from the CUDA Event specialization instead of checking the generic header guard at the include site. Signed-off-by: Jonathan Wong <131846439+nvjonwong@users.noreply.github.com>
Add the Experimental Event type and space_depends_on function to kokkos.core so C++20 module users can use the Event tests through import kokkos.core. Signed-off-by: Jonathan Wong <131846439+nvjonwong@users.noreply.github.com>
Signed-off-by: Christian Trott <crtrott@sandia.gov>
There was a problem hiding this comment.
Code Review
This pull request introduces an experimental Event API (Kokkos::Experimental::Event) to support fine-grained stream dependencies, providing a native implementation for the CUDA backend and a portable fallback for other backends. The review comments identify several critical issues that need to be addressed: a compilation error in the CUDA event resource due to mismatched member variable names, potential infinite loops in the fallback implementation due to a lack of volatile qualifiers on the flag view, incorrect usage of default-constructed execution spaces instead of the passed arguments, and a side effect in the CUDA backend where the active device is changed without being restored.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
|
||
| template <> | ||
| struct EventResource<Kokkos::Cuda> { | ||
| std::string label = "unknown"; |
There was a problem hiding this comment.
The member variable is declared as label, but the constructor initializes m_label and the label() method returns m_handle->m_label. This will cause a compilation error because m_label is undeclared. It should be renamed to m_label.
| std::string label = "unknown"; | |
| std::string m_label = "unknown"; |
| const Kokkos::View<int, Kokkos::SharedHostPinnedSpace>& flag_, | ||
| const ExecutionSpace& exec_) | ||
| : label(label_), flag(flag_), exec(exec_) {} | ||
| std::string label; | ||
| Kokkos::View<int, Kokkos::SharedHostPinnedSpace> flag; |
There was a problem hiding this comment.
The flag view is used for busy-waiting on the host in fence(). Without declaring the view's value type as volatile (or using volatile memory traits), the compiler is free to cache the value of flag() in a register during the loop, leading to an infinite loop/hang in optimized builds. Changing the value type to volatile int ensures that a volatile load is performed on every iteration.
const Kokkos::View<volatile int, Kokkos::SharedHostPinnedSpace>& flag_,
const ExecutionSpace& exec_)
: label(label_), flag(flag_), exec(exec_) {}
std::string label;
Kokkos::View<volatile int, Kokkos::SharedHostPinnedSpace> flag;| private: | ||
| using resource_t = Kokkos::Impl::EventResource<execution_space>; | ||
| using handle_t = std::shared_ptr<resource_t>; | ||
| using flag_t = Kokkos::View<int, Kokkos::SharedHostPinnedSpace>; |
There was a problem hiding this comment.
| Event(const std::string& label_, const execution_space& exec_space) | ||
| : m_handle(std::make_shared<resource_t>( | ||
| label_, | ||
| Kokkos::View<int, Kokkos::SharedHostPinnedSpace>( | ||
| std::string("Kokkos::Event::flag:") + label_), | ||
| execution_space())) { | ||
| record(exec_space); | ||
| }; |
There was a problem hiding this comment.
There are two issues here:
- The constructor default-constructs the execution space (
execution_space()) instead of using the passedexec_spaceparameter, which breaks instance-specific behavior. - It repeats the
Kokkos::Viewtype instead of using the definedflag_talias.
Using flag_t and passing exec_space correctly resolves both issues.
Event(const std::string& label_, const execution_space& exec_space)
: m_handle(std::make_shared<resource_t>(
label_,
flag_t(std::string("Kokkos::Event::flag:") + label_),
exec_space)) {
record(exec_space);
};| void record(const execution_space& exec_space) { | ||
| m_handle->flag() = 0; | ||
| m_handle->exec = execution_space(); |
There was a problem hiding this comment.
The record function default-constructs the execution space (execution_space()) and assigns it to m_handle->exec instead of using the passed exec_space parameter. This discards the specific execution space instance passed by the caller.
void record(const execution_space& exec_space) {
m_handle->flag() = 0;
m_handle->exec = exec_space;| explicit EventResource(const std::string& label, | ||
| const Kokkos::Cuda& exec_space) | ||
| : m_label(label), m_cudaDev(exec_space.cuda_device()) { | ||
| KOKKOS_IMPL_CUDA_SAFE_CALL(cudaSetDevice(m_cudaDev)); | ||
| KOKKOS_IMPL_CUDA_SAFE_CALL( | ||
| cudaEventCreateWithFlags(&m_event, cudaEventDisableTiming)); | ||
| } |
There was a problem hiding this comment.
Calling cudaSetDevice has the side effect of permanently changing the active CUDA device of the calling thread, which is not restored. This can lead to subtle bugs in subsequent CUDA operations in the same thread. It is safer to query the current device and restore it after creating the event.
explicit EventResource(const std::string& label,
const Kokkos::Cuda& exec_space)
: m_label(label), m_cudaDev(exec_space.cuda_device()) {
int current_dev = -1;
KOKKOS_IMPL_CUDA_SAFE_CALL(cudaGetDevice(¤t_dev));
KOKKOS_IMPL_CUDA_SAFE_CALL(cudaSetDevice(m_cudaDev));
KOKKOS_IMPL_CUDA_SAFE_CALL(
cudaEventCreateWithFlags(&m_event, cudaEventDisableTiming));
KOKKOS_IMPL_CUDA_SAFE_CALL(cudaSetDevice(current_dev));
}
Automated mirror of upstream PR kokkos#9245 This is a follow on to this PR: kokkos#9122 introducing a better fallback implementation.