-
Notifications
You must be signed in to change notification settings - Fork 80
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
mem_channel extension test plan #727
Conversation
I have some questions for @GarveyJoe, who I think is the original author of this extension:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
Co-authored-by: Ronan Keryell <ronan@keryell.fr>
Co-authored-by: Ronan Keryell <ronan@keryell.fr>
I believe our implementation responds to the aspect query by telling you if the device does something useful with the mem_channel property as opposed to telling you whether the device supports the extension (which all our devices can trivially support as the property is a hint). I think we can remove the aspect, but I think we're internally relying on it in the runtime so we'll have to change that code to not use the aspect.
Yes, uint32_t would be fine. This is a relic from when the OpenCL types were defined in the SYCL spec, but since they no longer are, this should be updated. |
@gmlueck, ping. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test plan looks OK, so I approved it.
We should make some changes to the extension spec (and implementation), though:
- Remove use of
cl_uint
from the API and useuint32_t
instead. - Either deprecate or remove the
ext_intel_mem_channel
aspect.
@GarveyJoe is this something your team can do soon, or should we open an internal tracker for this?
Created test plan for mem_channel extension.