-
Notifications
You must be signed in to change notification settings - Fork 770
[SYCL] Key/Value sorting with fixed-size private array input #14399
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
665a5c8
to
af2b274
Compare
af2b274
to
a968af6
Compare
Tag @andreyfe1 |
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.
Looks good to me. Thanks!
#endif | ||
|
||
template <typename T1, typename T2> class key_value_iterator { |
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'd suggest to move this iterator to a separate file since it does not semantically fit to group sort itself
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.
Ok, moved to a separate file.
void> | ||
sort_key_value_over_group(Group group, sycl::span<T, ElementsPerWorkItem> keys, | ||
sycl::span<U, ElementsPerWorkItem> values, | ||
ArraySorter array_sorter, |
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.
Let's keep the same type name as in Spec: Sorter
. Here we work with spans, not arrays
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.
Ok, renamed to Sorter.
ElementsPerWorkItem>::value, | ||
void> | ||
sort_key_value_over_group( | ||
experimental::group_with_scratchpad<Group, Extent> exec, |
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.
according to the Spec (and in ideal world) we want to support all group helpers, not only group_with_scratchpad
. So, let's keep support for all group helpers in general
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.
Changed to support all group helpers.
Regarding the comment here: #13942 (comment) |
This PR adds support for oneDPL zip iterator. Unfortunately I can't add a test for that to this repo because it will require creating a dependency from OneDPL. |
|
||
} // namespace detail | ||
} // namespace _V1 | ||
} // namespace sycl |
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.
nit: add empty line at the EOF
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.
Thank you, will have to move extension doc from proposed to supported in another PR, will make this correction there.
Implementation of sort_key_value_over_group APIs from
https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/proposed/sycl_ext_oneapi_group_sort.asciidoc#functions-with-fixed-size-arrays