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

Remove command-buffer command handle ref counting #2578

Merged
merged 3 commits into from
Jan 29, 2025

Conversation

EwanC
Copy link
Contributor

@EwanC EwanC commented Jan 16, 2025

The handles to individual commands returned from command-buffer append entry-points currently need reference counted by the user. However this isn't a model that maps to OpenCL/CUDA/HIP/Level-Zero where the lifetime of commands is tied to the lifetime of the parent command-buffer/graph/command-list.

Remove this idiom from the command-buffer UR API, adapters, and CTS. The APIs that are removed are:

  • urCommandBufferCommandGetInfoExp
  • urCommandBufferReleaseCommandExp
  • urCommandBufferRetainCommandExp

DPC++ PR - intel/llvm#16670

@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 opencl OpenCL adapter specific issues native-cpu Native CPU adapter specific issues command-buffer Command Buffer feature addition/changes/specification labels Jan 16, 2025
@EwanC EwanC force-pushed the ewan/remove_command_ref_counting branch from be8b4fc to ef1de22 Compare January 17, 2025 09:30
EwanC added a commit to EwanC/llvm that referenced this pull request Jan 17, 2025
Reflects UR change from oneapi-src/unified-runtime#2578
where we no longer need to manually manage the lifetimes of the
individual commands in a UR command-buffer.
EwanC added a commit to EwanC/llvm that referenced this pull request Jan 17, 2025
Reflects UR change from oneapi-src/unified-runtime#2578
where we no longer need to manually manage the lifetimes of the
individual commands in a UR command-buffer.
@EwanC EwanC force-pushed the ewan/remove_command_ref_counting branch from ef1de22 to 460eac2 Compare January 20, 2025 09:14
@EwanC EwanC marked this pull request as ready for review January 20, 2025 10:32
@EwanC EwanC requested review from a team as code owners January 20, 2025 10:32
Copy link
Contributor

@steffenlarsen steffenlarsen left a comment

Choose a reason for hiding this comment

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

LGTM!

@EwanC EwanC force-pushed the ewan/remove_command_ref_counting branch from 460eac2 to 49fb7a9 Compare January 22, 2025 15:04
@EwanC EwanC force-pushed the ewan/remove_command_ref_counting branch from 49fb7a9 to 924569c Compare January 22, 2025 16:08
@EwanC EwanC force-pushed the ewan/remove_command_ref_counting branch from 924569c to eb23fe5 Compare January 23, 2025 09:32
@EwanC
Copy link
Contributor Author

EwanC commented Jan 23, 2025

ping @oneapi-src/unified-runtime-native-cpu-write & @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 Jan 27, 2025
@kbenzie
Copy link
Contributor

kbenzie commented Jan 28, 2025

@EwanC please pull in the changes from main branch and run generate to resolve the conflict.

@EwanC EwanC force-pushed the ewan/remove_command_ref_counting branch from eb23fe5 to 7fba037 Compare January 28, 2025 15:45
EwanC added 2 commits January 29, 2025 09:49
The handles to individual commands returned from command-buffer
append entry-points currently need reference counted by the user.
However this isn't a model that maps to OpenCL/CUDA/HIP/Level-Zero
where the lifetime of commands is tied to the lifetime of the parent
command-buffer/graph/command-list.

Remove this idiom from the command-buffer UR API, adapters, and CTS.
The APIs that are removed are
* `urCommandBufferCommandGetInfoExp`
* `urCommandBufferReleaseCommandExp`
* `urCommandBufferRetainCommandExp`
@EwanC EwanC force-pushed the ewan/remove_command_ref_counting branch from 7fba037 to df1de2d Compare January 29, 2025 09:50
@EwanC
Copy link
Contributor Author

EwanC commented Jan 29, 2025

@EwanC please pull in the changes from main branch and run generate to resolve the conflict.

Done, thanks for the heads-up on the conflict

@kbenzie kbenzie merged commit 14f4a3b into oneapi-src:main Jan 29, 2025
9 checks passed
EwanC added a commit to EwanC/llvm that referenced this pull request Jan 29, 2025
Reflects UR change from oneapi-src/unified-runtime#2578
where we no longer need to manually manage the lifetimes of the
individual commands in a UR command-buffer.
martygrant pushed a commit to intel/llvm that referenced this pull request Jan 29, 2025
Reflects UR change from
oneapi-src/unified-runtime#2578 where we no
longer need to manually manage the lifetimes of the individual commands
in a UR command-buffer.
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 opencl OpenCL 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.

7 participants