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

[EXP][OpenCL] Update kernel commands in a command-buffer #1358

Merged
merged 2 commits into from
Apr 23, 2024

Conversation

EwanC
Copy link
Contributor

@EwanC EwanC commented Feb 19, 2024

Implement the API for updating the kernel commands in a command-buffer defined by #1089 for the OpenCL adapter.

However, the following changes to the UR kernel update API have been made based on implementation experience:

  1. Forbid updating the work-dim of the kernel, see Allow changes to work dim in cl_khr_command_buffer_mutable_dispatch  KhronosGroup/OpenCL-Docs#1057
  2. Remove struct fields to update exec info, after DPC++ implementation prototype shows this isn't needed.
  3. Forbid changing the local work size from user to impl defined and vice-versa. See discussion in L0 implementation PR.

This adapter implementation depends on support for the cl_khr_command_buffer_mutable_dispatch extension.

Tested on Intel GPU/CPUs OpenCL implementations with the command-buffer emulation layer.

$ OPENCL_LAYERS=<path/to/SimpleOpenCLSamples/build/layers/10_cmdbufemu/libCmdBufEmu.so> ./bin/test-exp_command_buffer --platform="Intel(R) OpenCL Graphics"

DPC++ PR intel/llvm#12724

@EwanC EwanC force-pushed the ewan/mutable_dispatch_update branch from 67da5d8 to 8def359 Compare February 19, 2024 13:19
@EwanC EwanC changed the title Ewan/mutable dispatch update [EXP][OpenCL] Update kernel commands in a command-buffer Feb 19, 2024
@EwanC EwanC force-pushed the ewan/mutable_dispatch_update branch from 8def359 to 9f5fe08 Compare February 19, 2024 13:24
@EwanC EwanC marked this pull request as ready for review February 19, 2024 14:31
@EwanC EwanC requested review from a team as code owners February 19, 2024 14:31
Copy link
Contributor

@martygrant martygrant left a comment

Choose a reason for hiding this comment

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

LGTM with a couple minor comments!

source/adapters/opencl/command_buffer.cpp Outdated Show resolved Hide resolved
source/adapters/opencl/command_buffer.cpp Outdated Show resolved Hide resolved
@EwanC EwanC force-pushed the ewan/mutable_dispatch_update branch from 9f5fe08 to ede570c Compare February 22, 2024 12:41
@codecov-commenter
Copy link

codecov-commenter commented Feb 22, 2024

Codecov Report

Attention: Patch coverage is 0% with 59 lines in your changes are missing coverage. Please review.

Project coverage is 12.43%. Comparing base (78ef1ca) to head (df68355).
Report is 201 commits behind head on main.

Files Patch % Lines
.../conformance/exp_command_buffer/invalid_update.cpp 0.00% 36 Missing ⚠️
.../conformance/exp_command_buffer/ndrange_update.cpp 0.00% 14 Missing ⚠️
...e/exp_command_buffer/buffer_fill_kernel_update.cpp 0.00% 9 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1358      +/-   ##
==========================================
- Coverage   14.82%   12.43%   -2.39%     
==========================================
  Files         250      241       -9     
  Lines       36220    36230      +10     
  Branches     4094     4112      +18     
==========================================
- Hits         5369     4506     -863     
- Misses      30800    31720     +920     
+ Partials       51        4      -47     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@EwanC EwanC force-pushed the ewan/mutable_dispatch_update branch 2 times, most recently from 3ceb6d9 to 5eeb6cf Compare February 27, 2024 17:41
@EwanC EwanC marked this pull request as draft March 14, 2024 18:20
@EwanC EwanC force-pushed the ewan/mutable_dispatch_update branch from 5eeb6cf to a442cbf Compare March 14, 2024 18:20
@EwanC EwanC marked this pull request as ready for review March 18, 2024 08:14
@EwanC EwanC force-pushed the ewan/mutable_dispatch_update branch from a442cbf to df68355 Compare March 25, 2024 09:48
Copy link
Contributor

@Bensuo Bensuo 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/mutable_dispatch_update branch 2 times, most recently from 7419c39 to 3ba6673 Compare April 5, 2024 10:15
@EwanC EwanC added the ready to merge Added to PR's which are ready to merge label Apr 5, 2024
@kbenzie kbenzie added conformance Conformance test suite issues. specification Changes or additions to the 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 experimental Experimental feature additions/changes/specification labels Apr 10, 2024
@kbenzie kbenzie added the command-buffer Command Buffer feature addition/changes/specification label Apr 16, 2024
Implement the API for updating the kernel commands in a command-buffer
defined by oneapi-src#1089 for
the OpenCL adapter.

However, the following changes to the UR kernel update API have been
made based on implementation experience:

1. Forbid updating the work-dim of the kernel, see KhronosGroup/OpenCL-Docs#1057
2. Remove struct fields to update exec info, after [DPC++ implementation
   prototype](intel/llvm#12840) shows this isn't
   needed.
3. Forbid changing the local work size from user to impl defined and
   vice-versa. See discussion in [L0 implementation
PR](oneapi-src#1353 (comment)).

This adapter implementation depends on support for the
[cl_khr_command_buffer_mutable_dispatch](https://registry.khronos.org/OpenCL/specs/3.0-unified/html/OpenCL_Ext.html#cl_khr_command_buffer_mutable_dispatch)
extension.

Tested on Intel GPU/CPUs OpenCL implementations with the
[command-buffer emulation
layer](https://github.com/bashbaug/SimpleOpenCLSamples/tree/main/layers/10_cmdbufemu).

```bash
$ OPENCL_LAYERS=<path/to/SimpleOpenCLSamples/build/layers/10_cmdbufemu/libCmdBufEmu.so> ./bin/test-exp_command_buffer --platform="Intel(R) OpenCL Graphics"
```

DPC++ PR intel/llvm#12724
@EwanC EwanC force-pushed the ewan/mutable_dispatch_update branch from 3ba6673 to 1449fa1 Compare April 22, 2024 14:59
@github-actions github-actions bot added the loader Loader related feature/bug label Apr 22, 2024
@EwanC EwanC force-pushed the ewan/mutable_dispatch_update branch from 1449fa1 to c3c5406 Compare April 22, 2024 17:06
@kbenzie kbenzie merged commit 85bc7b7 into oneapi-src:main Apr 23, 2024
51 checks passed
sommerlukas pushed a commit to intel/llvm that referenced this pull request Apr 24, 2024
Test the UR commit that enables updating kernel commands in a
command-buffer in the OpenCL adapter
oneapi-src/unified-runtime#1358
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.

5 participants