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.

Sign up for GitHub

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

Jump to bottom

[OpenCL] Add Command Buffer extension to OpenCL adapter. #966

Merged
merged 2 commits into from
Nov 22, 2023
Merged

[OpenCL] Add Command Buffer extension to OpenCL adapter. #966

merged 2 commits into from
Nov 22, 2023

Conversation

martygrant
Copy link
Contributor

@martygrant martygrant commented Oct 17, 2023 •
edited
Loading

Adds implementations for the experimental Command Buffer entry points. Also fixes minor build error with missing namespace in enqueue.cpp.

Added companion intel-llvm PR which also updates many of the E2E tests to run against OpenCL - intel/llvm#11718

@pbalcer pbalcer mentioned this pull request Oct 17, 2023
Merged
@martygrant martygrant marked this pull request as ready for review October 17, 2023 14:16
@martygrant martygrant requested a review from a team as a code owner October 17, 2023 14:16
EwanC
EwanC reviewed Oct 18, 2023
View reviewed changes
source/adapters/opencl/command_buffer.cpp Outdated Show resolved Hide resolved
source/adapters/opencl/command_buffer.cpp Outdated Show resolved Hide resolved
source/adapters/opencl/command_buffer.cpp Outdated Show resolved Hide resolved
source/adapters/opencl/command_buffer.cpp Outdated Show resolved Hide resolved
source/adapters/opencl/command_buffer.hpp Outdated Show resolved Hide resolved
source/adapters/opencl/common.hpp Show resolved Hide resolved
source/adapters/opencl/command_buffer.cpp Outdated Show resolved Hide resolved
@martygrant martygrant force-pushed the martin/openclCommandBuffers branch 2 times, most recently from 683f0da to 489854e Compare October 18, 2023 13:57
@kbenzie kbenzie changed the title [SYCL][OpenCL] Add Command Buffer extension to OpenCL adapter. [OpenCL] Add Command Buffer extension to OpenCL adapter. Oct 18, 2023
EwanC
EwanC reviewed Oct 18, 2023
View reviewed changes
source/adapters/opencl/command_buffer.cpp Outdated Show resolved Hide resolved
source/adapters/opencl/command_buffer.cpp Outdated Show resolved Hide resolved
@martygrant martygrant force-pushed the martin/openclCommandBuffers branch 3 times, most recently from 6333136 to 31632c6 Compare October 18, 2023 15:38
fabiomestre
fabiomestre approved these changes Oct 18, 2023
View reviewed changes
source/adapters/opencl/command_buffer.cpp Outdated Show resolved Hide resolved
EwanC
EwanC requested changes Oct 19, 2023
View reviewed changes
Copy link
Contributor

@EwanC EwanC left a comment

Choose a reason for hiding this comment

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

This code LGTM, but I think this PR should have a companion change in DPC++. Once this UR commit get's pulled into DPC++ the SYCL-Graph extension will start reporting support for OpenCL with undocumented behaviour. Also without DPC++ testing, i'm not sure how to use this code path, given that I don't think there's UR CTS testing for experimental features.

What i'd imagine the companion DPC++ PR to cover:

  • Add a OpenCL section to the design doc for SYCL-Graphs https://github.com/intel/llvm/blob/sycl/sycl/doc/design/CommandGraph.md#backend-implementation documenting the implementation on cl_khr_command_buffer, and explicitly documenting the types of nodes that aren't currently supported.
  • Add opencl to the REQUIRES line of test-e2e/Graph tests which now pass with this change.
  • Add a device.get_info<exp_ext::info::device::graph_support>(); check to all tests, skipping returning early for those which don't report support. As even for an OpenCL device, support is dependent on the cl_khr_command_buffer extension being available. This will probably be the case for all DPC++ CI testing, which I don't think uses a device with cl_khr_command_buffer support.

@martygrant
Copy link
Contributor Author

Hey @EwanC, thanks for the really thorough review. That all sounds fine for a new PR into DPC++, I'll open up an issue with the details you've provided. Should one PR be merged before the other?

We're temporarily holding off merging any more stuff into the adapters branch here until a L0 issue is reverted but I'll have both of these changes ready to go once that's sorted.

@EwanC
Copy link
Contributor

EwanC commented Oct 19, 2023

Hey @EwanC, thanks for the really thorough review. That all sounds fine for a new PR into DPC++, I'll open up an issue with the details you've provided. Should one PR be merged before the other?

We're temporarily holding off merging any more stuff into the adapters branch here until a L0 issue is reverted but I'll have both of these changes ready to go once that's sorted.

I think following the existing adapter change process is fine https://oneapi-src.github.io/unified-runtime/core/CONTRIB.html#adapter-change-process This will need to be merged first, but only after the DPC++ PR has been approved and CI passing.

@martygrant martygrant mentioned this pull request Oct 19, 2023
Closed
@martygrant
Copy link
Contributor Author

Have opened up an issue for the work required in DPC++ - intel/llvm#11599

@martygrant martygrant requested a review from a team as a code owner October 31, 2023 10:23
This was referenced Oct 31, 2023
Closed
Merged
Merged
@martygrant
Copy link
Contributor Author

Corrected an error in urCommandBufferAppendKernelLaunchExp where the cl_command_queue command_queue parameter for clCommandNDRangeKernelKHR must be NULL. Given the extension function pointers more meaningful names instead of just F.

EwanC
EwanC approved these changes Nov 14, 2023
View reviewed changes
Copy link
Contributor

@EwanC EwanC left a comment

Choose a reason for hiding this comment

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

LGTM 😃

@kbenzie kbenzie merged commit 109ed46 into oneapi-src:adapters Nov 22, 2023
48 checks passed
sergey-semenov pushed a commit to intel/llvm that referenced this pull request Nov 28, 2023
@martygrant @reble
@EwanC @kbenzie
[SYCL][OpenCL] Enable graph extension on OpenCL backend (#11718)
5d75245 
intel-llvm CI run for adding Command Buffers to the OpenCL Adapter in
Unified Runtime - oneapi-src/unified-runtime#966

Also completes follow-on work identified in #11599 to add an OpenCL
section to the SYCL-Graphs docs and update the e2e Graph tests. Updating
the tests has since been completed in a separate PR -
#11877

Depends on #11820 merging first.

---------

Co-authored-by: Pablo Reble <pablo@reble.org>
Co-authored-by: Ewan Crawford <ewan@codeplay.com>
Co-authored-by: Kenneth Benzie (Benie) <k.benzie@codeplay.com>
callumfare pushed a commit to kbenzie/llvm that referenced this pull request Dec 18, 2023
@martygrant @reble
@EwanC @kbenzie @callumfare
[SYCL][OpenCL] Enable graph extension on OpenCL backend (intel#11718)
795871c 
intel-llvm CI run for adding Command Buffers to the OpenCL Adapter in
Unified Runtime - oneapi-src/unified-runtime#966

Also completes follow-on work identified in intel#11599 to add an OpenCL
section to the SYCL-Graphs docs and update the e2e Graph tests. Updating
the tests has since been completed in a separate PR -
intel#11877

Depends on intel#11820 merging first.

---------

Co-authored-by: Pablo Reble <pablo@reble.org>
Co-authored-by: Ewan Crawford <ewan@codeplay.com>
Co-authored-by: Kenneth Benzie (Benie) <k.benzie@codeplay.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kbenzie kbenzie kbenzie left review comments

@EwanC EwanC EwanC approved these changes

@fabiomestre fabiomestre fabiomestre approved these changes

@veselypeta veselypeta veselypeta approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@martygrant @EwanC @kbenzie @fabiomestre @veselypeta