-
Notifications
You must be signed in to change notification settings - Fork 787
[SYCL] Follow up fixes for group_sort extension #14591
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
* Change memory_required queries to return the exact size of local memory which needs to be allocated for group_sort algorithm, currently it is a bit more than required. * Fix a bug in align_key_value_scratch where I didn't take into account that std::align changes value of KeysScratchSpace. To find scratch of the values I need to use original value of this variable. * Fix mistake in the test where keys/values were mixed up.
|
tag @andreyfe1 |
How did it pass before and how did you find it? Should we change some values for keys/values in the test as well? |
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.
Thanks!
This is more of a cosmetic change. So it doesn't actually matter if we do:
or
Keys and Data vectors are filled with random numbers (and these two vectors are different) and test serves it purpose no matter the order of the aforementioned arguments, so additional changes are not necessary. |
Please add this explanation to the PR description. |
sycl/include/sycl/ext/oneapi/experimental/group_helpers_sorters.hpp
Outdated
Show resolved
Hide resolved
Added. |
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.
LGTM for two out of three changes here. Still needs @dm-vodopyanov 's approval.
Suggest to approve without my approval because code reviews for all previous |
memory_required
queries to return the exact size of local memory which needs to be allocated forgroup_sort
algorithm, currently it is a bit more than required.align_key_value_scratch
where I didn't take into account thatstd::align
changes value ofKeysScratchSpace
. To find scratch of the values I need to use original value of this variable.RunSortKeyValueOverGroup functions accepts two vectors - the first is considered keys, and the second is considered values.
Verification is done in the same function (with the same assumption that the first vector is keys and the second is values).
So it doesn't actually matter in which order aforementioned arguments are provided, test still serves it purpose.