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

Enable the registry of level_zero_v2 #2571

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Enable the registry of level_zero_v2 #2571

wants to merge 1 commit into from

Conversation

omarahmed1111
Copy link
Contributor

This PR enable us to register level_zero_v2 in the list of UR adapters, so it could be used with dpc++.

@omarahmed1111 omarahmed1111 requested a review from a team as a code owner January 16, 2025 10:22
@github-actions github-actions bot added the loader Loader related feature/bug label Jan 16, 2025
@pbalcer
Copy link
Contributor

pbalcer commented Jan 16, 2025

The issue with this is that SYCL will now see every L0 device twice, so applications would have to be able to select one or the other.
I'm not opposed, since this is the simplest approach to exposing the v2 adapter, but I think we should gate this behind an env variable, e.g.:
UR_ADAPTER_LEVEL_ZERO_V2=1. If set, only v2 adapter is loaded, else we only load current adapter.

@igchor ping

@omarahmed1111 omarahmed1111 force-pushed the enable-registry-of-level_zero_v2 branch from 5568592 to 6a790d4 Compare January 16, 2025 11:16
@pbalcer pbalcer mentioned this pull request Jan 16, 2025
Open
@omarahmed1111 omarahmed1111 force-pushed the enable-registry-of-level_zero_v2 branch from 6a790d4 to bbf7e01 Compare January 16, 2025 14:47
@github-actions github-actions bot added the specification Changes or additions to the specification label Jan 16, 2025
pbalcer
pbalcer approved these changes Jan 16, 2025
View reviewed changes
Copy link
Contributor

@pbalcer pbalcer left a comment

Choose a reason for hiding this comment

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

lgtm, although long-term I think we should think how we can achieve the same thing using ONEAPI_DEVICE_SELECTOR.

@kbenzie
Copy link
Contributor

kbenzie commented Jan 16, 2025

lgtm, although long-term I think we should think how we can achieve the same thing using ONEAPI_DEVICE_SELECTOR.

The implementation in UR is not currently functional, unfortunately.

@omarahmed1111 omarahmed1111 added the ready to merge Added to PR's which are ready to merge label Jan 16, 2025
@kbenzie kbenzie removed the ready to merge Added to PR's which are ready to merge label Jan 17, 2025
@kbenzie
Copy link
Contributor

kbenzie commented Jan 17, 2025

I've taken the ready to merge tag off this, add it back again when intel/llvm#16656 is approved.

@omarahmed1111 omarahmed1111 force-pushed the enable-registry-of-level_zero_v2 branch 2 times, most recently from 58ccaa9 to 765d4fd Compare January 29, 2025 14:00
@omarahmed1111 omarahmed1111 force-pushed the enable-registry-of-level_zero_v2 branch from 765d4fd to 5ce773d Compare January 29, 2025 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@igchor igchor igchor approved these changes

@pbalcer pbalcer pbalcer approved these changes

Assignees
No one assigned
Labels
loader Loader related feature/bug specification Changes or additions to the specification
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@omarahmed1111 @pbalcer @kbenzie @igchor