Fix fallback implementation for sort_by_key #6856
Conversation
masterleinad
force-pushed
the
fix_fallbacksort_by_key
branch
from
a98b8e4
to
65ba68c
Compare
| template <class ExecutionSpace, class Layout> | ||
| struct sort_on_device : std::false_type {}; |
There was a problem hiding this comment.
Why bother with the trait? It doesn't seem like you use it. You can directly specialize the variable template.
| // For the fallback sort_by_key implementation we might need to execute | ||
| // Kokkos::sort on the host. This type trait determines at compile-time where we |
There was a problem hiding this comment.
we might need to execute Kokkos::sort on the host
"We need to consider if Kokkos::sort defers to the fallback implementation that copies the array to the host and uses std::sort. The value is determined by the implementation choices in the impl/Kokkos_SortImpl.hpp which mean they need to be kept in sync (sigh...)"
| // For the fallback implementation for sort_by_key using Kokkos::sort, we need | ||
| // to consider if Kokkos::sort defers to the fallback implementation that copies | ||
| // the array to the host and uses std::sort. This decision is made in | ||
| // impl/Kokkos_SortImpl.hpp and this type trait decides at runtime when it is |
There was a problem hiding this comment.
this type trait decides at runtime when it is safe to assume that Kokkos::sort doesn't manually copy to the host
I don't understand this sentence. There is no more trait and the variable template does not "decide" anything. Do you mean it is used somewhere to determine what to do?
There was a problem hiding this comment.
We manually copy all data to the host and provide sort with a host execution space if that variable is false. Otherwise, we assume that sort doesn't copy to the host and we don't need to copy anything manually.
There was a problem hiding this comment.
Please improve/update the comment
|
Retest this please |
|
Only the |
|
Ignoring |
* Fix fallback implementation for sort_by_key * Guard with KOKKOS_ENABLE_ONEDPL * Drop sort_on_device * Improve wording * Improve comment
Alternative to #6849. For the fallback implementation of
sort_by_keyusingKokkos::sort, we need to know in advance if the sort executes on the same device or the host so we can copy the keys used in the comparator accordingly.This pull request makes this decision explicitly and we only call
sorton the device in cases where we are sure that it doesn't execute in the host. Note that this disregards runtime decisions like when usingSYCLwithLayoutStrideand we just play it safe by executing on the host in this case. Making it a compile-time decision might improve compile-time and avoids dealing with SYCL device-copyable issues in both branches.