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

Add test plan for oneapi_non_uniform_groups extension #866

Conversation

steffenlarsen
Copy link
Contributor

This commit adds a test plan for the oneapi_non_uniform_groups oneAPI extension.

This commit adds a test plan for the
[oneapi_non_uniform_groups](https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/experimental/sycl_ext_oneapi_non_uniform_groups.asciidoc)
oneAPI extension.

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
@steffenlarsen steffenlarsen requested review from gmlueck and a team as code owners February 6, 2024 14:32
Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
@bader bader requested a review from keryell February 8, 2024 16:13
@gmlueck
Copy link
Contributor

gmlueck commented Feb 14, 2024

@Pennycook could I delegate my review role to you for this test plan? I think you'd have better insight since you were the original author of this extension spec. (I tried adding you in the "Reviewers" section, but for some reason GitHub doesn't find you there.)

@Pennycook
Copy link
Contributor

@Pennycook could I delegate my review role to you for this test plan? I think you'd have better insight since you were the original author of this extension spec. (I tried adding you in the "Reviewers" section, but for some reason GitHub doesn't find you there.)

I think I can't be officially added as a reviewer because I'm not associated with the SYCL-CTS repository. Regardless, I'll take a look!

test_plans/non_uniform_groups.asciidoc Show resolved Hide resolved
test_plans/non_uniform_groups.asciidoc Outdated Show resolved Hide resolved
test_plans/non_uniform_groups.asciidoc Show resolved Hide resolved
test_plans/non_uniform_groups.asciidoc Show resolved Hide resolved
test_plans/non_uniform_groups.asciidoc Show resolved Hide resolved
test_plans/non_uniform_groups.asciidoc Outdated Show resolved Hide resolved
test_plans/non_uniform_groups.asciidoc Outdated Show resolved Hide resolved
test_plans/non_uniform_groups.asciidoc Outdated Show resolved Hide resolved
test_plans/non_uniform_groups.asciidoc Outdated Show resolved Hide resolved
test_plans/non_uniform_groups.asciidoc Show resolved Hide resolved
Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
@bader bader requested a review from Pennycook February 20, 2024 18:19
Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
steffenlarsen added a commit to steffenlarsen/SYCL-CTS that referenced this pull request Feb 21, 2024
This commit adds tests for the [sycl_ext_oneapi_non_uniform_groups](https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/experimental/sycl_ext_oneapi_non_uniform_groups.asciidoc)
extension in accordance with the test plan currently in review here:
KhronosGroup#866
@steffenlarsen
Copy link
Contributor Author

Draft PR with the tests: #875

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
Copy link
Contributor

@Pennycook Pennycook left a comment

Choose a reason for hiding this comment

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

Typo aside, LGTM!

Co-authored-by: John Pennycook <john.pennycook@intel.com>
==== get_group_id

Check that `get_group_id()` return type is `id_type` and return value is
`1` if the predicate was `true` or `0` if the predicate was `false`.
Copy link
Member

Choose a reason for hiding this comment

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

It confused me, but this is what the specification says.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ironically, the implementation and extension specification slightly disagree on this. I have a patch to make them agree: intel/llvm#12905


=== The `fixed_size_group` class API

Let `N` be some power-of-two value greater than 1 that is expected to be a
Copy link
Member

Choose a reason for hiding this comment

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

Are you excluding the case of 1. That seems interesting to test corner cases, otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the class API tests we only test for one group configuration. The intention here was to pick one that wasn't 1 to avoid some trivial cases and to test a more normal group size. We could potentially run the tests for multiple configurations, if that would be more interesting? We do it for the group algorithms.

Copy link
Member

Choose a reason for hiding this comment

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

Oh I thought you were running with several N and in that case testing also the boundaries would have made sense.

@bader bader requested a review from keryell March 14, 2024 15:06
@bader
Copy link
Contributor

bader commented Mar 14, 2024

@keryell, could you please approve the PR if all your concerns are addressed?

@bader bader closed this Mar 14, 2024
@bader bader reopened this Mar 14, 2024
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.

That looks interesting!

@bader bader merged commit 01c8aa1 into KhronosGroup:SYCL-2020 Mar 21, 2024
15 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