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 incorrect all_of validation when sub-group size is 1 #795

Merged
merged 1 commit into from
Sep 7, 2023

Conversation

frasercrmck
Copy link
Contributor

When the sub-group size is 1, the 'one_true' and 'some_true' predicates return true, so we should instead be checking that all_of returns true.

@frasercrmck frasercrmck requested a review from a team as a code owner September 6, 2023 17:40
@CLAassistant
Copy link

CLAassistant commented Sep 6, 2023

CLA assistant check
All committers have signed the CLA.

@frasercrmck
Copy link
Contributor Author

frasercrmck commented Sep 6, 2023

This problem technically also applies to the corresponding work-group tests, but I'm assuming we'll never run those tests with a work-group size of 1.

Copy link
Contributor

@bader bader left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this!

I'm assuming we'll never run those tests with a work-group size of 1.

Probably it's worth adding an assertion for that + a comment which of existing checks not work for a work-group size of 1.

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.

Good catch!
Could you add a comment in front of all your changes explaining the problem?

@frasercrmck
Copy link
Contributor Author

Thanks for comments, will address later in the day.

I have another question, though. I was looking at this again with your feedback in mind, but is it valid for the predicate_function_of_sub_group test to be using get_global_linear_id as it does? Should we not be using the sub-group local linear ID ?

I ask because I thought it was invalid to make assumptions about the mapping of work-items to sub-group items. The predicate_function_of_sub_group test seems to rely on the first sub-group with ID 0 having all of the work-item IDs from 0 to (sub-group-size)-1. The bool_function_of_sub_group test doesn't do this, as it runs on all work-items of all sub-groups and uses the sub-group linear ID.

But also (noticed while writing this comment) is the bool_function_of_sub_group not overwriting its results for every sub-group? It seems to run over all sub-groups but only write to a fixed set of indices. Does that mean that some sub-groups might report incorrect results but get ignored?

@frasercrmck
Copy link
Contributor Author

I've updated with the comments requested.

@keryell
Copy link
Member

keryell commented Sep 7, 2023

@frasercrmck do you plan to dive into your discoveries in your last comment?

@frasercrmck
Copy link
Contributor Author

@frasercrmck do you plan to dive into your discoveries in your last comment?

If you agree they're real issues, yeah I can certainly take a look. I would prefer to keep that to a separate task/PR though if that's alright; I might need some guidance and a bit more back-and-forth as I'm not actually very that used to writing SYCL. Is that okay?

When the sub-group size is 1, the 'one_true' and 'some_true' predicates
return true, so we should instead be checking that all_of returns true.
@bader bader merged commit a515668 into KhronosGroup:SYCL-2020 Sep 7, 2023
7 checks passed
@frasercrmck frasercrmck deleted the fix-subgroup-size-1 branch September 7, 2023 17:47
@keryell
Copy link
Member

keryell commented Sep 7, 2023

@frasercrmck do you plan to dive into your discoveries in your last comment?

If you agree they're real issues, yeah I can certainly take a look. I would prefer to keep that to a separate task/PR though if that's alright; I might need some guidance and a bit more back-and-forth as I'm not actually very that used to writing SYCL. Is that okay?

sub-groups are very confusing to me. After looking at the SYCL specification, I have the feeling that the problem you expose is real.
T local_var(item.get_global_linear_id() + 1); should be T local_var = sub_group.get_local_linear_id()
and so on.

frasercrmck added a commit to frasercrmck/SYCL-CTS that referenced this pull request Sep 27, 2023
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.
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.

4 participants