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

PI_USM_INDIRECT_ACCESS exec info is unconditionally set #11970

Closed
Naghasan opened this issue Nov 21, 2023 · 3 comments
Closed

PI_USM_INDIRECT_ACCESS exec info is unconditionally set #11970

Naghasan opened this issue Nov 21, 2023 · 3 comments
Labels
bug Something isn't working

Comments

@Naghasan
Copy link
Contributor

Right after the creation of a kernel, piKernelSetExecInfo is unconditionally called to set PI_USM_INDIRECT_ACCESS. However not all kernels require this as they may purely use accessor, making this call unnecessary.

For an underlying runtime supporting USM this has no (or shouldn't have) implication. But if the runtime does not support it, this artificially raises the bar as it now has to support an unnecessary extension.

@Naghasan Naghasan added the bug Something isn't working label Nov 21, 2023
@AlexeySachkov
Copy link
Contributor

For an underlying runtime supporting USM this has no (or shouldn't have) implication.

I'm not sure if that is the case for at least OpenCL, for example: cl_intel_unified_shared_memory. If I understand correctly, it does not guarantees that a USM pointer passed within a struct (and not directly as a kernel argument through clSetKernelArgMemPointerINTEL is accessible unless corresponding exec mode is set.

But if the runtime does not support it, this artificially raises the bar as it now has to support an unnecessary extension.

With that I agree. We could change the semantic of PI_USM_INDIRECT_ACCESS for it to mean "set if supported", but that is likely a hack. The proper solution is likely to first check if indirect access is supported and only then set the property

@steffenlarsen
Copy link
Contributor

@Naghasan - Did your patch (#12780) address this issue? If yes, can we close this?

@Naghasan
Copy link
Contributor Author

Naghasan commented May 7, 2024

yes it does, thanks for flagging it up

@Naghasan Naghasan closed this as completed May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants