[SYCL] Add support for key/value sorting APIs #13942
Conversation
| range_size, comp, | ||
| aligned_scratch_ptr + range_size * sizeof(T)); | ||
| val = *local_copy; | ||
| } |
There was a problem hiding this comment.
Should we add something like assert(false && "Scratch size less than value results in UB") in the else part? It will be helpful in debugging.
There was a problem hiding this comment.
Unfortunately native assert is not supported by all devices and I believe fallback assert can add overhead (as it creates assert buffer and copies it back) and I'm not sure if it is reliable enough to use it in default execution path. That's why I didn't put assert here.
|
Tag @andreyfe1 |
| : comp(comp_), scratch(scratch_) {} | ||
|
|
||
| template <typename Group> | ||
| T operator()([[maybe_unused]] Group g, [[maybe_unused]] T val) { |
There was a problem hiding this comment.
Is it planned to add another operator(): void operator()(Group g, sycl::span<T, ElementsPerWorkItem> values, Properties properties); ?
The same for group_key_value_sorter and radix_sorters
There was a problem hiding this comment.
I didn't add them on purpose to split functionality into several PRs. Do you think it is better to create a single PR with all functionality?
There was a problem hiding this comment.
No, I needed to make sure that it's not forgotten. I'm fine if it's covered in a separate PR
againull
force-pushed
the
feature/key_value_scalar
branch
from
a6187e6 to
6aa7995
Compare
|
@uditagarwal97 @aelovikov-intel This PR was rebased on top of the latest changes and is ready for review. |
| auto this_comp = this->comp; | ||
| auto comp_key_value = [this_comp](const KeyValue &lhs, |
There was a problem hiding this comment.
| auto this_comp = this->comp; | |
| auto comp_key_value = [this_comp](const KeyValue &lhs, | |
| auto comp_key_value = [this_comp = this->comp](const KeyValue &lhs, |
| @@ -343,6 +343,41 @@ class group_sorter { | |||
| } | |||
| }; | |||
|
|
|||
| template <typename T, typename U, typename CompareT = std::less<>, | |||
There was a problem hiding this comment.
Please rename T/U to KeyTy/ValueTy.
| typename std::is_same<std::tuple<Key, Value>, | ||
| decltype(std::declval<Sorter>()( | ||
| std::declval<G>(), std::declval<Key>(), | ||
| std::declval<Value>()))>; |
There was a problem hiding this comment.
I think std::invoke_result_t would allow to simplify this a bit.
There was a problem hiding this comment.
Thanks, simplified.
| g, KeyValue(key, value)); | ||
| } | ||
|
|
||
| static constexpr std::size_t memory_required(sycl::memory_scope scope, |
There was a problem hiding this comment.
According to the spec memory_required is not constexpr for default_sorters. Please, make corresponding changes for all default sorters
| @@ -343,6 +343,41 @@ class group_sorter { | |||
| } | |||
There was a problem hiding this comment.
This comment is regarding https://github.com/intel/llvm/blob/sycl/sycl/include/sycl/ext/oneapi/experimental/group_helpers_sorters.hpp#L252
It's expected for joint_sorter (default and radix sorters both) to support oneDPL zip_iterator (https://github.com/intel/llvm/blob/sycl/sycl/include/sycl/ext/oneapi/experimental/group_helpers_sorters.hpp#L252 : Example 3).
Do you plan to add this support (no matter in this PR or a separate one)?
There was a problem hiding this comment.
I will look into this but not as part of this PR.
Add group_key_value_sorter sorters and sort_key_value_over_group APIs based on https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/proposed/sycl_ext_oneapi_group_sort.asciidoc extension. This PR was split out from larger PR: intel#13713 Co-authored-by: "Andrei Fedorov [[email protected]](mailto:[email protected])" Co-authored-by: "Romanov Vlad [[email protected]](mailto:[email protected])"
Add group_key_value_sorter sorters and sort_key_value_over_group APIs based on https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/proposed/sycl_ext_oneapi_group_sort.asciidoc extension.
This PR was split out from larger PR: #13713
Co-authored-by: "Andrei Fedorov [email protected]"
Co-authored-by: "Romanov Vlad [email protected]"