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

Remove the spec requirement for adapters to bounds check USM operations. #1060

Closed

Conversation

aarongreig
Copy link
Contributor

None of the adapter APIs support this kind of bounds checking natively so mandating it in our spec means mandating an apparently unnecessary additional overhead in the adapters.

Addresses #926

None of the adapter APIs support this kind of bounds checking natively
so mandating it in our spec means mandating an apparently unnecessary
additional overhead in the adapters.
@fabiomestre
Copy link
Contributor

fabiomestre commented Nov 9, 2023

Why are we removing this checks for USM but not for Buffers? Is it that much harder to do the check on USM entrypoints?

I would expect them to be similar in that we need to track the initial allocation of memory and the size of the buffer / usm memory in the validation layers.

@aarongreig
Copy link
Contributor Author

the native APIs do generally have bounds checking for buffer operations, the adapters don't need to do anything for that

@aarongreig
Copy link
Contributor Author

...maybe that's only true for OpenCL actually

@fabiomestre
Copy link
Contributor

fabiomestre commented Nov 9, 2023

...maybe that's only true for OpenCL actually

Yeah. Both Hip and Cuda rely on the same functions for buffer and usm operations (and I don't think either of them has this kind of bound check natively). Not entirely sure about level zero.

My feeling about this is that we should either support it for both USM and Buffers using validation layers which avoids adding extra load on the adapters. Or not support it at all.

@@ -6619,7 +6619,6 @@ urEnqueueMemUnmap(
/// + `patternSize > size`
/// + `(patternSize & (patternSize - 1)) != 0`
/// + `size % patternSize != 0`
/// + If `size` is higher than the allocation size of `ptr`

Choose a reason for hiding this comment

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

this is not supported by the adapters, but it could be added to the validation layer as common functionality for all. It would be a matter of querying for the allocations' size and compare against the size passed.

@aarongreig
Copy link
Contributor Author

...maybe that's only true for OpenCL actually

Yeah. Both Hip and Cuda rely on the same functions for buffer and usm operations (and I don't think either of them has this kind of bound check natively). Not entirely sure about level zero.

My feeling about this is that we should either support it for both USM and Buffers using validation layers which avoids adding extra load on the adapters. Or not support it at all.

yep agreed, in light of every adapter that isn't OpenCL we should just move the checks into the validation layer instead of removing the requirement from the spec

@aarongreig aarongreig closed this Nov 9, 2023
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