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

[CL] Add UR handles to OpenCL adapter #1176

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

[CL] Add UR handles to OpenCL adapter #1176

wants to merge 10 commits into from

Conversation

omarahmed1111
Copy link
Contributor

@omarahmed1111 omarahmed1111 commented Dec 11, 2023 •
edited by kbenzie
Loading

Redesign OpenCL adapter by adding UR handles and get rid of the casting of the UR handles to OPENCL handles.

intel/llvm#12172

@codecov-commenter
Copy link

codecov-commenter commented Dec 11, 2023 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 12.43%. Comparing base (78ef1ca) to head (97dbfd0).
Report is 199 commits behind head on main.

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

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1176      +/-   ##
==========================================
- Coverage   14.82%   12.43%   -2.40%     
==========================================
  Files         250      241       -9     
  Lines       36220    36242      +22     
  Branches     4094     4111      +17     
==========================================
- Hits         5369     4506     -863     
- Misses      30800    31732     +932     
+ Partials       51        4      -47

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

@omarahmed1111 omarahmed1111 force-pushed the Add-handles-to-opencl-adapter branch 3 times, most recently from ceb8b8e to ff5ebc1 Compare December 14, 2023 10:45
@omarahmed1111 omarahmed1111 mentioned this pull request Dec 14, 2023
Draft
@omarahmed1111 omarahmed1111 changed the title [OPENCL] Redesign FuncPtrExtCache by adding UR handles to OPENCL adapter [OPENCL] Add UR handles to OPENCL adapter Dec 14, 2023
@omarahmed1111 omarahmed1111 force-pushed the Add-handles-to-opencl-adapter branch 10 times, most recently from f7b0de5 to 66d5282 Compare December 20, 2023 15:40
@omarahmed1111 omarahmed1111 force-pushed the Add-handles-to-opencl-adapter branch 2 times, most recently from adb6b3d to 4777b93 Compare December 22, 2023 23:05
@omarahmed1111 omarahmed1111 force-pushed the Add-handles-to-opencl-adapter branch 3 times, most recently from 11b092b to ff193b2 Compare January 12, 2024 13:56
@omarahmed1111 omarahmed1111 marked this pull request as ready for review January 12, 2024 14:34
@omarahmed1111 omarahmed1111 requested review from a team as code owners January 12, 2024 14:34
@omarahmed1111 omarahmed1111 force-pushed the Add-handles-to-opencl-adapter branch 2 times, most recently from ab61e80 to 61ba805 Compare January 16, 2024 11:23
@kbenzie kbenzie added the opencl OpenCL adapter specific issues label Apr 10, 2024
@github-actions github-actions bot added conformance Conformance test suite issues. command-buffer Command Buffer feature addition/changes/specification labels Oct 10, 2024
@aarongreig aarongreig mentioned this pull request Oct 15, 2024
Open
@omarahmed1111 omarahmed1111 force-pushed the Add-handles-to-opencl-adapter branch 2 times, most recently from f56cf10 to 7252088 Compare October 16, 2024 14:43
kbenzie
kbenzie reviewed Oct 17, 2024
View reviewed changes
source/adapters/opencl/context.hpp Outdated Show resolved Hide resolved
source/adapters/opencl/context.hpp Outdated Show resolved Hide resolved
source/adapters/opencl/device.hpp
Comment on lines +42 to +44
uint32_t incrementReferenceCount() noexcept { return ++RefCount; }

uint32_t decrementReferenceCount() noexcept { return --RefCount; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Reference count and associated member functions is something we should provide a shared abstraction for. That's out of scope for this PR, though. Seeing this just reminded me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, good point, that would eliminate a lot of repetition.

Copy link
Contributor

Choose a reason for hiding this comment

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

Something like this already exists in at least the Native CPU adapter. I'm currently figuring out where this should live as the laoder also has reference counted objects.

source/adapters/opencl/device.hpp
Comment on lines +78 to +77
ur_result_t checkDeviceExtensions(const std::vector<std::string> &Exts,
bool &Supported) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This interface isn't very nice to use. Do we really need to return the ur_result_t here? Could it just return false?

This is another issue that can be looked at separately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't look into this area much but I think this could be addressed in a follow-up that would redesign these extensions helper functions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense.

source/adapters/opencl/enqueue.cpp Outdated Show resolved Hide resolved
source/adapters/opencl/enqueue.cpp Outdated Show resolved Hide resolved
source/adapters/opencl/platform.cpp Outdated Show resolved Hide resolved
source/adapters/opencl/queue.hpp Outdated Show resolved Hide resolved
@kbenzie kbenzie changed the title [OPENCL] Add UR handles to OPENCL adapter [CL] Add UR handles to OpenCL adapter Oct 22, 2024
@omarahmed1111 omarahmed1111 force-pushed the Add-handles-to-opencl-adapter branch 2 times, most recently from e24d912 to d8f0aef Compare November 6, 2024 17:16
@kbenzie kbenzie self-assigned this Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aarongreig aarongreig aarongreig approved these changes

@Bensuo Bensuo Bensuo approved these changes

@fabiomestre fabiomestre fabiomestre approved these changes

@kbenzie kbenzie kbenzie approved these changes

Assignees

@kbenzie kbenzie

Labels
command-buffer Command Buffer feature addition/changes/specification conformance Conformance test suite issues. opencl OpenCL adapter specific issues
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@omarahmed1111 @codecov-commenter @kbenzie @fabiomestre @Bensuo @aarongreig