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] Don't set PI_USM_INDIRECT_ACCESS if platform don't support it #12780

Merged

Conversation

Naghasan
Copy link
Contributor

If the OpenCL platform doesn't support USM, don't set PI_USM_INDIRECT_ACCESS exec info. This will avoid SYCL program to fail when they don't use USM. If the program do need USM support, the runtime will fail on other API calls (like memory allocation).

If the OpenCL platform doesn't support USM, don't set
PI_USM_INDIRECT_ACCESS exec info. This will avoid SYCL program to fail
when they don't use USM. If the program do need USM support, the runtime will fail
on other API calls (like memory allocation).

Signed-off-by: Victor Lomuller <victor@codeplay.com>
@Naghasan
Copy link
Contributor Author

A few points:

  • I'm not too sure how to create a test for that, I'm happy to try suggestions if you have any
  • another way to do this would be to return 'unsupported' from UR and the SYCL runtime to log it but not fail. I had an offline chat with @kbenzie and he suggested this approach for now

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.

Overall looks good. Just a small comment and then we need the CMake changes to point to the right repo again. 👍

sycl/source/detail/kernel_impl.cpp Show resolved Hide resolved
@Naghasan
Copy link
Contributor Author

@steffenlarsen After discussion with Beni, I cut the link to the UR patch (will be caught with another bump). So if you are happy you can approve it, no risk to make it point to the wrong repo now :)

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.

Looks good!

@Naghasan
Copy link
Contributor Author

@intel/llvm-gatekeepers Ready to merge (failing CI job is unrelated and common to other PRs)

@sommerlukas sommerlukas merged commit 375e579 into intel:sycl Feb 27, 2024
10 of 11 checks passed
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.

3 participants