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

Fix sub-group 'of' tests #807

Merged
merged 5 commits into from
Nov 9, 2023
Merged

Conversation

frasercrmck
Copy link
Contributor

As discovered/discussed in
#795, the predicate_function_of_sub_group test was using the global linear ID to perform sub-group 'of' checks.

The checks were only being run on the first sub-group (with ID 0). This was implicitly relying on the first sub-group containing the work-items with IDs from 0 up to the sub-group size, as the checks included conditions like exactly one ID being 1. This is not correct in general, as the mapping between work-items and sub-group items is not defined.

In addition, this commit also enhances the sub-group tests to check that all sub-groups in the work-group return the correct result. Before we were either checking only the first sub-group
(predicate_function_of_sub_group) or checking all sub-groups but overwriting the results each time, so in effect only checking the last sub-group.

Now the results are initialized to 'true' and each sub-group is run and its results are merged with all of the other results.

As discovered/discussed in
KhronosGroup#795, the
`predicate_function_of_sub_group` test was using the global linear ID to
perform sub-group 'of' checks.

The checks were only being run on the first sub-group (with ID 0). This
was implicitly relying on the first sub-group containing the work-items
with IDs from 0 up to the sub-group size, as the checks included
conditions like exactly one ID being 1. This is not correct in general,
as the mapping between work-items and sub-group items is not defined.

In addition, this commit also enhances the sub-group tests to check that
*all* sub-groups in the work-group return the correct result. Before we
were either checking only the *first* sub-group
(`predicate_function_of_sub_group`) or checking all sub-groups but
overwriting the results each time, so in effect only checking the *last*
sub-group.

Now the results are initialized to 'true' and each sub-group is run and
its results are merged with all of the other results.
@frasercrmck frasercrmck requested a review from a team as a code owner September 27, 2023 09:51
@keryell
Copy link
Member

keryell commented Sep 28, 2023

That sounds good. Probably apply some of #804 (comment)
Do not forget that when we read the code in the future we do not have access to the PR description.

@frasercrmck
Copy link
Contributor Author

That sounds good. Probably apply some of #804 (comment) Do not forget that when we read the code in the future we do not have access to the PR description.

I've added some extra comments now. I'm not sure exactly what suggestions you were making, so hopefully this is what you had in mind.

@bader bader requested review from keryell and a team November 4, 2023 00:46
@bader bader added the help wanted Extra attention is needed label Nov 4, 2023
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.

This is quite clearer! \o/

tests/group_functions/group_of.h Outdated Show resolved Hide resolved
tests/group_functions/group_of.h Outdated Show resolved Hide resolved
@bader bader requested a review from keryell November 8, 2023 16:40
@frasercrmck
Copy link
Contributor Author

I don't suppose someone would be able/willing to squash this branch when merging? It's become a little messy (at least to my mind). No worries either way, though.

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. I prefer keeping the history even it is messy, otherwise this is no longer history. :-)

@bader bader merged commit 2374893 into KhronosGroup:SYCL-2020 Nov 9, 2023
7 checks passed
@frasercrmck frasercrmck deleted the fix-sub-group-of branch November 14, 2023 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants