Skip to content

[UR] Add handles to opencl adapter #17572

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
Merged
kbenzie merged 1 commit into from
Apr 4, 2025
Merged

[UR] Add handles to opencl adapter #17572

kbenzie merged 1 commit into from
Apr 4, 2025

Conversation

@RossBrunton
Copy link
Contributor

@RossBrunton RossBrunton commented Mar 21, 2025

No description provided.

@RossBrunton
Copy link
Contributor Author

RossBrunton commented Mar 21, 2025

This was from oneapi-src/unified-runtime#1176 . It flagged up some issues in LLVM which needed to be fixed (which they have now been).

@RossBrunton RossBrunton force-pushed the ross/clhandles branch 2 times, most recently from 68d37bb to 6a7908a Compare March 25, 2025 16:40
@RossBrunton RossBrunton marked this pull request as ready for review March 26, 2025 11:17
@RossBrunton RossBrunton requested review from a team as code owners March 26, 2025 11:17
aarongreig
aarongreig reviewed Mar 27, 2025
View reviewed changes
aarongreig
aarongreig reviewed Mar 27, 2025
View reviewed changes
aarongreig
aarongreig reviewed Mar 27, 2025
View reviewed changes
EwanC
EwanC reviewed Mar 27, 2025
View reviewed changes
EwanC
EwanC reviewed Mar 31, 2025
View reviewed changes
aarongreig
aarongreig reviewed Apr 1, 2025
View reviewed changes
Copy link
Contributor

@aarongreig aarongreig left a comment

Choose a reason for hiding this comment

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

looks good, just a couple more points

@RossBrunton
Copy link
Contributor Author

RossBrunton commented Apr 3, 2025

@intel/llvm-reviewers-runtime There are two small fixes to the runtime here:

  • There was an incorrect cast of a UR handle -> CL handle, which is no longer valid.
  • urKernelCreateWithNativeHandle now actively does some validation on OpenCL, so the correct return type needed to be set.

@RossBrunton
Copy link
Contributor Author

RossBrunton commented Apr 4, 2025

@intel/llvm-gatekeepers Please merge.

@kbenzie kbenzie merged commit 42ee42e into intel:sycl Apr 4, 2025
35 of 44 checks passed
@coldav coldav mentioned this pull request Apr 17, 2025
Closed
@kbenzie kbenzie mentioned this pull request May 5, 2025
Merged
kbenzie added a commit that referenced this pull request May 14, 2025
@kbenzie
[SYCL][CL] Fix ownership of native handle in sycl::make_queue (#18309)
8cd13a7 
Level Zero is the only backend which has any explicit ownership transfer
controls when creating SYCL objects from native handles.

The CUDA and HIP backends already had the default to not transferring
ownership of native handles.

Up until this patch the OpenCL backend was transferring ownership when
creating with a native handle, when combined with the changes to the
OpenCL adapter in #17572 this caused
crashes in existing workloads which expect to manage ownership of the
OpenCL native handles.

Fixes URT-905.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@uditagarwal97 uditagarwal97 uditagarwal97 approved these changes

+2 more reviewers

@EwanC EwanC EwanC approved these changes

@aarongreig aarongreig aarongreig approved these changes

Reviewers whose approvals may not affect merge requirements

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

@RossBrunton @EwanC @uditagarwal97 @aarongreig @kbenzie