-
Notifications
You must be signed in to change notification settings - Fork 769
[SYCL] Renaming fixed_size_group to chunk #15721
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
Conversation
sycl/include/sycl/detail/spirv.hpp
Outdated
@@ -76,10 +76,10 @@ struct is_ballot_group< | |||
sycl::ext::oneapi::experimental::ballot_group<ParentGroup>> | |||
: std::true_type {}; | |||
|
|||
template <typename Group> struct is_fixed_size_group : std::false_type {}; | |||
template <typename Group> struct is_chunk : std::false_type {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that we already have this type trait defined in type_traits.hpp
, let's to an #include
instead of redefining that trait
sycl/include/sycl/detail/spirv.hpp
Outdated
@@ -1303,7 +1303,7 @@ ControlBarrier(Group g, memory_scope FenceScope, memory_order Order) { | |||
template <__spv::GroupOperation Op, size_t PartitionSize, \ | |||
typename ParentGroup, typename T> \ | |||
inline T Group##Instruction( \ | |||
ext::oneapi::experimental::fixed_size_group<PartitionSize, ParentGroup> \ | |||
ext::oneapi::experimental::chunk<PartitionSize, ParentGroup> \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Formatting is broken here
fixed_size_group<PartitionSize, Group>> | ||
get_fixed_size_group(Group group); | ||
chunk<PartitionSize, Group>> | ||
get_chunk(Group group); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sycl_ext_oneapi_non_uniform_groups
does not define get_chunk
, I suppose you meant chunked_partition
|
||
template <size_t PartitionSize, typename ParentGroup> class fixed_size_group { | ||
template <size_t PartitionSize, typename ParentGroup> class chunk { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also leave a TODO
comment to implement operator fragment<ParentGroup>() const;
once fragment
is ready
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably rename the file as well. Also applies to fixed_size_group_algorithms.cpp
|
||
template <size_t PartitionSize, typename ParentGroup> class fixed_size_group { | ||
template <size_t PartitionSize, typename ParentGroup> class chunk { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
template <size_t PartitionSize, typename ParentGroup> class chunk { | |
template <size_t ChunkSize, typename ParentGroup> class chunk { |
That's rather minor, but I think that it is better to be aligned in the terminology we use
I am concerned about doing the changes in #14604 gradually. I do not mind having multiple patches for them, but they should go in around the same time. We also need to make sure we coordinate these changes with changes to https://github.com/KhronosGroup/SYCL-CTS/tree/SYCL-2020/tests/extension/oneapi_non_uniform_groups. |
First part of changes in order of #14604:
fixed_size_group -> chunk