Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add event update to command-buffers #1823

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

EwanC
Copy link
Contributor

@EwanC EwanC commented Jul 4, 2024

Expand the command-buffer experimental feature API so that it can be used to implement SYCL-Graph dynamic
events
.

This involves extending each command append entry-point to include the following extra parameters:

  • An output ur_exp_command_buffer_command_handle_t.
  • An Input ur_event_handle_t event wait-list of dependent events.
  • An output ur_event_handle_t event that is signaled when the command completes its next execution.

New entry-points are also added to update the wait-list and signal event parameters of commands:

  • urCommandBufferUpdateSignalEventExp
  • urCommandBufferUpdateWaitEventsExp

The UR changes are implemented for the cuda adapter only in this PR to prove the concepts, and other adapter implementations will follow as subsequent PRs to ease review.

New CTS tests are also added, see exp_command_buffer/event_sync_kernel_command.cpp and exp_command_buffer/event_sync.cpp

DPC++ PR intel/llvm#14459

@EwanC EwanC force-pushed the ewan/ur_dyn_events branch 2 times, most recently from e27e2de to 6afed3f Compare July 4, 2024 15:28
@github-actions github-actions bot added loader Loader related feature/bug conformance Conformance test suite issues. specification Changes or additions to the specification experimental Experimental feature additions/changes/specification level-zero L0 adapter specific issues cuda CUDA adapter specific issues hip HIP adapter specific issues native-cpu Native CPU adapter specific issues command-buffer Command Buffer feature addition/changes/specification labels Jul 4, 2024
EwanC added a commit to reble/llvm that referenced this pull request Jul 9, 2024
@EwanC EwanC force-pushed the ewan/ur_dyn_events branch 3 times, most recently from a480a67 to 4c3389f Compare July 9, 2024 11:43
EwanC added a commit to reble/llvm that referenced this pull request Jul 9, 2024
EwanC added a commit to reble/llvm that referenced this pull request Jul 9, 2024
@EwanC EwanC force-pushed the ewan/ur_dyn_events branch 2 times, most recently from c559851 to 938cbe4 Compare July 10, 2024 09:23
@EwanC EwanC force-pushed the ewan/ur_dyn_events branch 3 times, most recently from e8db775 to ddf3f10 Compare July 30, 2024 09:28
@EwanC EwanC force-pushed the ewan/ur_dyn_events branch 3 times, most recently from af9b056 to ea35fb5 Compare August 6, 2024 13:05
@EwanC EwanC requested a review from a team as a code owner August 23, 2024 09:09
@EwanC EwanC requested a review from hdelan August 23, 2024 09:09
@EwanC EwanC force-pushed the ewan/ur_dyn_events branch 2 times, most recently from bcb101b to 98a6f5e Compare September 2, 2024 07:48
Copy link
Contributor

@hdelan hdelan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly LGTM but a few small questions. Most importantly about checking whether profiling is enabled or not.

source/adapters/cuda/command_buffer.cpp Show resolved Hide resolved
source/adapters/cuda/command_buffer.cpp Show resolved Hide resolved
source/adapters/cuda/command_buffer.cpp Show resolved Hide resolved
@EwanC EwanC force-pushed the ewan/ur_dyn_events branch 3 times, most recently from cea3086 to 28d5122 Compare September 18, 2024 13:45
EwanC added a commit to reble/llvm that referenced this pull request Oct 2, 2024
EwanC added a commit to reble/llvm that referenced this pull request Oct 2, 2024
EwanC added a commit to reble/llvm that referenced this pull request Oct 2, 2024
Copy link
Contributor

@hdelan hdelan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@PietroGhg PietroGhg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Native CPU lgtm, thank you

@EwanC EwanC force-pushed the ewan/ur_dyn_events branch 2 times, most recently from 6550981 to 23d2720 Compare October 4, 2024 08:29
scripts/core/EXP-COMMAND-BUFFER.rst Outdated Show resolved Hide resolved
scripts/core/exp-command-buffer.yml Outdated Show resolved Hide resolved
source/adapters/cuda/command_buffer.cpp Outdated Show resolved Hide resolved
test/conformance/exp_command_buffer/fixtures.h Outdated Show resolved Hide resolved
Expand the command-buffer experimental feature API so that
it can be used to implement [SYCL-Graph dynamic
events](reble/llvm#372).

This involves extending each command append entry-point to
include the following extra parameters:
  * An output `ur_exp_command_buffer_command_handle_t`.
  * An Input `ur_event_handle_t` event wait-list of dependent events.
  * An output `ur_event_handle_t` event that is signaled when the command
    completes its next execution.

New entry-points are also added to update the wait-list and signal event
parameters of commands:
  * `urCommandBufferUpdateSignalEventExp`
  * `urCommandBufferUpdateWaitEventsExp`

APIs implemented for CUDA adapter with CTS tests.
@EwanC
Copy link
Contributor Author

EwanC commented Oct 7, 2024

ping @oneapi-src/unified-runtime-level-zero-write for review

@EwanC EwanC added the ready to merge Added to PR's which are ready to merge label Oct 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
command-buffer Command Buffer feature addition/changes/specification conformance Conformance test suite issues. cuda CUDA adapter specific issues experimental Experimental feature additions/changes/specification hip HIP adapter specific issues level-zero L0 adapter specific issues loader Loader related feature/bug native-cpu Native CPU adapter specific issues ready to merge Added to PR's which are ready to merge specification Changes or additions to the specification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants