Skip to content
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

sort: single API entry point for basic overloads and move code to impl #6239

Merged
merged 6 commits into from
Jun 30, 2023

Conversation

fnrizzi
Copy link
Contributor

@fnrizzi fnrizzi commented Jun 23, 2023

Another step towards the refactoring and a stepping stone for adding the custom comparator in a separate PR.
This PR moves code from the public API to the impl such that these lower-level functions can be reused if needed, and to have a single public API entry point where it is easier to reason from.

Before

template <class ExecutionSpace, class DataType, class... Properties>
std::enable_if_t<(Kokkos::is_execution_space<ExecutionSpace>::value) &&
                 (!SpaceAccessibility<
                     HostSpace, typename Kokkos::View<DataType, Properties...>::
                                    memory_space>::accessible)>
sort(const ExecutionSpace& exec, const Kokkos::View<DataType, Properties...>& view){
  // call binsort
}

#if defined(KOKKOS_ENABLE_ONEDPL)
template <class DataType, class... Properties>
void sort(const Experimental::SYCL& space, const Kokkos::View<DataType, Properties...>& view)
{
  // sort via onedpl
}
#endif

template <class ExecutionSpace, class DataType, class... Properties>
std::enable_if_t<(Kokkos::is_execution_space<ExecutionSpace>::value) &&
                 (SpaceAccessibility<
                     HostSpace, typename Kokkos::View<DataType, Properties...>::
                                    memory_space>::accessible)>
sort(const ExecutionSpace&, const Kokkos::View<DataType, Properties...>& view)
{
  // call std sort
}

#if defined(KOKKOS_ENABLE_CUDA)
template <class DataType, class... Properties>
void sort(const Cuda& space, const Kokkos::View<DataType, Properties...>& view)
{
  // call thrust 
}
#endif

template <class ViewType>
void sort(ViewType const& view) {
  // call overload with view's exespace
}

With this PR

template <class ExecutionSpace, class DataType, class... Properties>
void sort([[maybe_unused]] const ExecutionSpace& exec,
          const Kokkos::View<DataType, Properties...>& view) 
{
  // constraints ...
  // ...
  // for now this is called for OpenMP, Threads, Serial and HPX
  if constexpr (Impl::better_off_calling_std_sort_v<ExecutionSpace>){
    auto first = ::Kokkos::Experimental::begin(view);
    auto last  = ::Kokkos::Experimental::end(view);
    std::sort(first, last);
  } else {
    Impl::sort_device_view_without_comparator(exec, view);
  }

template <class DataType, class... Properties>
void sort(const Kokkos::View<DataType, Properties...>& view) {
  // call overload with view's exespace
}

whre Impl::sort_device_view_without_comparator(exec, view); takes care of dispatching things to maintain same behavior as before but this is inside the impl namespace:

namespace Impl{

#if defined(KOKKOS_ENABLE_CUDA)
template <class DataType, class... Properties>
void sort_device_view_without_comparator(const Cuda& exec, 
                                         const Kokkos::View<DataType, Properties...>& view) {
  sort_cudathrust(exec, view);
}
#endif

#if defined(KOKKOS_ENABLE_ONEDPL)
template <class DataType, class... Properties>
void sort_device_view_without_comparator(const Experimental::SYCL& exec,
                                         const Kokkos::View<DataType, Properties...>& view) { 
  sort_onedpl(exec, view);
}
#endif

// fallback case
template <class ExecutionSpace, class DataType, class... Properties>
std::enable_if_t<Kokkos::is_execution_space<ExecutionSpace>::value>
sort_device_view_without_comparator(const ExecutionSpace& exec,
                                     const Kokkos::View<DataType, Properties...>& view) {
  sort_via_binsort(exec, view);
}

} //end namespace Impl

@fnrizzi fnrizzi changed the base branch from master to develop June 23, 2023 18:22
@fnrizzi fnrizzi closed this Jun 23, 2023
@fnrizzi fnrizzi reopened this Jun 23, 2023
@fnrizzi

This comment was marked as outdated.

algorithms/src/sorting/impl/Kokkos_SortImpl.hpp Outdated Show resolved Hide resolved
algorithms/src/sorting/impl/Kokkos_SortImpl.hpp Outdated Show resolved Hide resolved
algorithms/src/sorting/impl/Kokkos_SortImpl.hpp Outdated Show resolved Hide resolved
algorithms/src/sorting/Kokkos_SortPublicAPI.hpp Outdated Show resolved Hide resolved
algorithms/src/sorting/Kokkos_SortPublicAPI.hpp Outdated Show resolved Hide resolved
algorithms/src/sorting/Kokkos_SortPublicAPI.hpp Outdated Show resolved Hide resolved
algorithms/src/sorting/Kokkos_SortPublicAPI.hpp Outdated Show resolved Hide resolved
algorithms/src/sorting/impl/Kokkos_SortImpl.hpp Outdated Show resolved Hide resolved
algorithms/src/sorting/impl/Kokkos_SortImpl.hpp Outdated Show resolved Hide resolved
algorithms/src/sorting/impl/Kokkos_SortImpl.hpp Outdated Show resolved Hide resolved
@fnrizzi
Copy link
Contributor Author

fnrizzi commented Jun 28, 2023

ci failure unrelated:

The following tests FAILED:
	 45 - Kokkos_UnitTest_Random (Failed)

@crtrott crtrott merged commit 13948ec into kokkos:develop Jun 30, 2023
27 of 28 checks passed
@fnrizzi fnrizzi mentioned this pull request Oct 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants