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

Update reqd_sub_group_size argument name. #506

Merged
merged 1 commit into from
Dec 7, 2023

Conversation

bader
Copy link
Contributor

@bader bader commented Nov 29, 2023

dim name is tipically used to denote the dimension (see reqd_work_group_size attribute description). In this case we set the size characterisitic, rather than the dimension.

`dim` name is tipically used to denote the dimension (see reqd_work_group_size attribute description). In this case we set the size characterisitic, rather than the dimension.
@Pennycook
Copy link
Contributor

I think it was originally written as "dim" to match reqd_work_group_size.

Should we change to reqd_work_group_size(size0, size1, size2) as well? Note that what the attribute really sets in that case is the size of the work-group in each dimension, so I think the same logic could apply.

@bader
Copy link
Contributor Author

bader commented Nov 29, 2023

I think it was originally written as "dim" to match reqd_work_group_size.

Should we change to reqd_work_group_size(size0, size1, size2) as well? Note that what the attribute really sets in that case is the size of the work-group in each dimension, so I think the same logic could apply.

According to my understanding, the reqd_sub_group_size doesn't specify the size in any particular dimension.
E.g. sub-group size 8 can be implemented as group of work-items with dimension sizes (1, 1, 8) or (1, 2, 4) or (2, 2, 2).
Therefore, using dim for reqd_sub_group_size doesn't make sense.

I don't have a strong opinion about reqd_work_group_size, but reqd_work_group_size(size0, size1, size2) looks clearer to me than reqd_work_group_size(dim0, dim1, dim2).

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.

I agree that it makes sense to change reqd_sub_group_size because the parameter is not related to the dimensions of the invocation range.

I could go either way with the naming of reqd_work_group_size. The parameters here describe the size of the work-group in each dimension, so the words "size" and "dim" both make sense.

@gmlueck
Copy link
Contributor

gmlueck commented Dec 7, 2023

This needs one more reviewer. Can someone volunteer?

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.

Yes, that looks more uniform. Thanks.

@gmlueck gmlueck merged commit 299ac1c into KhronosGroup:SYCL-2020/master Dec 7, 2023
2 checks passed
keryell pushed a commit that referenced this pull request Sep 10, 2024
Update reqd_sub_group_size argument name.
gmlueck added a commit that referenced this pull request Nov 7, 2024
Update reqd_sub_group_size argument name.

(cherry picked from commit 299ac1c)
gmlueck added a commit that referenced this pull request Nov 7, 2024
Update reqd_sub_group_size argument name.

(cherry picked from commit 299ac1c)
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