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

mem_channel extension test plan #727

Merged
merged 6 commits into from
Jul 10, 2023

Conversation

AmirIpma
Copy link
Contributor

Created test plan for mem_channel extension.

@AmirIpma AmirIpma requested review from gmlueck and a team as code owners June 23, 2023 09:41
test_plans/mem_channel.asciidoc Outdated Show resolved Hide resolved
test_plans/mem_channel.asciidoc Outdated Show resolved Hide resolved
test_plans/mem_channel.asciidoc Outdated Show resolved Hide resolved
@AmirIpma AmirIpma requested a review from keryell June 26, 2023 12:36
@bader bader requested a review from gmlueck June 27, 2023 15:22
@gmlueck
Copy link
Contributor

gmlueck commented Jun 27, 2023

I have some questions for @GarveyJoe, who I think is the original author of this extension:

  • The extension says there is an aspect aspect::ext_intel_mem_channel. What does this aspect mean? Can we test it somehow in this test plan?

  • One of the comments above notes that it does not make sense for the extension to use the OpenCL C type cl_uint, and I agree. Can this be changed in the extension spec to either unsigned int or uint32_t?

Copy link
Member

@keryell keryell left a comment

Choose a reason for hiding this comment

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

Thanks.

test_plans/mem_channel.asciidoc Outdated Show resolved Hide resolved
test_plans/mem_channel.asciidoc Outdated Show resolved Hide resolved
AmirIpma and others added 2 commits June 29, 2023 12:58
Co-authored-by: Ronan Keryell <ronan@keryell.fr>
Co-authored-by: Ronan Keryell <ronan@keryell.fr>
@GarveyJoe
Copy link

The extension says there is an aspect aspect::ext_intel_mem_channel. What does this aspect mean? Can we test it somehow in this test plan?

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.

One of the comments above notes that it does not make sense for the extension to use the OpenCL C type cl_uint, and I agree. Can this be changed in the extension spec to either unsigned int or uint32_t?

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.

@bader
Copy link
Contributor

bader commented Jul 7, 2023

@gmlueck, ping.

Copy link
Contributor

@gmlueck gmlueck left a 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 use uint32_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?

@bader bader merged commit f753b5f into KhronosGroup:SYCL-2020 Jul 10, 2023
8 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.

5 participants