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

[SYCL][HIP] Add interop header and devcie specialization #7145

Merged
merged 1 commit into from
Nov 7, 2022

Conversation

npmiller
Copy link
Contributor

Fixes #6635 for HIP.

Note that it does mean the HIP extension header must be included in user applications in order to get the correct specialization, as is done in the test:

#include <sycl/ext/oneapi/backend/hip.hpp>

@npmiller npmiller requested a review from a team as a code owner October 21, 2022 15:57
@npmiller
Copy link
Contributor Author

I don't think the test failure is related, that test is marked unsupported on HIP and has nothing to do with interop

@densamoilov
Copy link

@npmiller, is there any reason why sycl/ext/oneapi/backend/hip.hpp must be included for HIP backend but CUDA backend doesn't need sycl/ext/oneapi/backend/cuda.hpp?

againull
againull previously approved these changes Nov 2, 2022
@againull againull dismissed their stale review November 2, 2022 20:15

Good concern from @densamoilov. I think we probably can define this specialization in backend.hpp header itself in the same way as it is done for other backends.

@againull againull self-requested a review November 2, 2022 20:16
@npmiller
Copy link
Contributor Author

npmiller commented Nov 7, 2022

There's pretty extensive discussions on what's going on for CUDA on this PR:

But essentially including the extension header is how specializations for the different backends is supposed to work. The only reason it's not for CUDA is that there are projects using it without the header AND we're planning on changing the CUDA interop interface at some point, so decided to hold off on fixing it to use the header until we're ready to update the interface as well. This is not currently the case for the HIP interop as far as I understand it.

@againull
Copy link
Contributor

againull commented Nov 7, 2022

There's pretty extensive discussions on what's going on for CUDA on this PR:

But essentially including the extension header is how specializations for the different backends is supposed to work. The only reason it's not for CUDA is that there are projects using it without the header AND we're planning on changing the CUDA interop interface at some point, so decided to hold off on fixing it to use the header until we're ready to update the interface as well. This is not currently the case for the HIP interop as far as I understand it.

Thank you for clarification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CUDA][HIP] get_native() API doesn't work for device
3 participants