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

[CUDA][HIP] Associate queue with device in context #1992

Merged
merged 3 commits into from
Sep 25, 2024

Conversation

hdelan
Copy link
Contributor

@hdelan hdelan commented Aug 19, 2024

Making a native queue doesn't require hDevice to be non null, but this associates the queue with a null device, even if hContext contains valid devices.

This fixes the behaviour for a single device context, making the queue be associated with the only device in the context. But there is a question mark over whether this entry point makes sense for a multi device UR context. Should we be able to construct a queue with a native queue without knowing what the UR device associated with the queue is? Probably not.

@hdelan hdelan marked this pull request as ready for review August 19, 2024 16:40
@hdelan hdelan requested review from a team as code owners August 19, 2024 16:40
@github-actions github-actions bot added cuda CUDA adapter specific issues hip HIP adapter specific issues labels Aug 19, 2024
@steffenlarsen
Copy link
Contributor

This fixes the behaviour for a single device context, making the queue be associated with the only device in the context. But there is a question mark over whether this entry point makes sense for a multi device UR context. Should we be able to construct a queue with a native queue without knowing what the UR device associated with the queue is? Probably not.

This is a weird case and maybe we need to consider what the expectation from the UR API is. In the L0 adapter it just picks the first device in the platform, not even caring if it is actually in the context.

@hdelan
Copy link
Contributor Author

hdelan commented Aug 20, 2024

This is a weird case and maybe we need to consider what the expectation from the UR API is.

I think that's a good idea. I am inclined to think that we shouldn't allow the hDevice param to be null so that we always have a valid device associated with the queue. We can optionally check if hContext contains hDevice but I think the context param is sort of meaningless.

In the L0 adapter it just picks the first device in the platform, not even caring if it is actually in the context.

That seems bad.

@rafbiels
Copy link
Contributor

rafbiels commented Sep 2, 2024

Could we separate the discussion about multi-device context from this single-device context fix? This PR brings back the (relatively uncontroversial) behaviour broken recently and we have a customer waiting for this fix.

@hdelan
Copy link
Contributor Author

hdelan commented Sep 2, 2024

Good idea. Sorry I have been sleeping on this one. I'll make an issue about this question and then hopefully we can merge this.

@hdelan
Copy link
Contributor Author

hdelan commented Sep 2, 2024

#2041

@hdelan
Copy link
Contributor Author

hdelan commented Sep 3, 2024

Ping @steffenlarsen are you happy to continue discussion about this entry point in the linked issue? If so are you happy with this as a quick-fix change?

Copy link
Contributor

@steffenlarsen steffenlarsen left a comment

Choose a reason for hiding this comment

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

Sure, let's do it!

Making a native queue doesn't require hDevice to be non null, but this
associates the queue with a null device, even if hContext contains valid
devices.

This fixes the behaviour for a single device context, making the queue
be associated with the only device in the context. But there is a
question mark over whether this entry point makes sense for a multi
device UR context. Should we be able to construct a queue with a native
queue without knowing what the UR device associated with the queue is?
Probably not.
@hdelan hdelan added the ready to merge Added to PR's which are ready to merge label Sep 4, 2024
@aarongreig aarongreig merged commit 2ad3268 into oneapi-src:main Sep 25, 2024
74 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cuda CUDA adapter specific issues hip HIP adapter specific issues ready to merge Added to PR's which are ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants