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.

Sign up for GitHub

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

Jump to bottom

Allow passing a non-trivially-copyable comparator to sort #1932

Merged
merged 3 commits into from
Nov 6, 2024
Merged

Allow passing a non-trivially-copyable comparator to sort #1932

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
71fe89c
Allow passing a device-copyable comparator to sort
dmitriy-sobolev Nov 4, 2024
1fa7438
Add sycl_traits.h
dmitriy-sobolev Nov 5, 2024
af9577c
More comments for device_copyable test
dmitriy-sobolev Nov 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions include/oneapi/dpl/pstl/hetero/dpcpp/parallel_backend_sycl_merge_sort.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
#include <type_traits> // std::decay_t, std::integral_constant

#include "sycl_defs.h" // __dpl_sycl::__local_accessor, __dpl_sycl::__group_barrier
#include "sycl_traits.h" // SYCL traits specialization for some oneDPL types.
#include "../../utils.h" // __dpl_bit_floor, __dpl_bit_ceil
#include "../../utils_ranges.h" // __difference_t
#include "parallel_backend_sycl_merge.h" // __find_start_point, __serial_merge
Expand Down
10 changes: 10 additions & 0 deletions include/oneapi/dpl/pstl/hetero/dpcpp/sycl_traits.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,9 @@ struct __write_to_id_if_else;
template <typename _ExecutionPolicy, typename _Pred>
struct __early_exit_find_or;

template <typename _Range, typename _Compare>
struct __leaf_sorter;

} // namespace oneapi::dpl::__par_backend_hetero

template <typename _UnaryOp>
Expand Down Expand Up @@ -313,6 +316,13 @@ struct sycl::is_device_copyable<_ONEDPL_SPECIALIZE_FOR(oneapi::dpl::__par_backen
{
};

template <typename _Range, typename _Compare>
struct sycl::is_device_copyable<_ONEDPL_SPECIALIZE_FOR(oneapi::dpl::__par_backend_hetero::__leaf_sorter, _Range,
_Compare)>
: oneapi::dpl::__internal::__are_all_device_copyable<_Range, _Compare>
{
};

namespace oneapi::dpl::unseq_backend
{

Expand Down
14 changes: 14 additions & 0 deletions test/general/implementation_details/device_copyable.pass.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,15 @@ test_device_copyable()
oneapi::dpl::__par_backend_hetero::__early_exit_find_or<policy_non_device_copyable, noop_device_copyable>>,
"__early_exit_find_or is not device copyable with device copyable types");

// __leaf_sorter
// Note that the use of noop_device_copyable/noop_non_device_copyable is valid in this context because
// sycl::is_device_copyable specialization for __leaf_sorter does not require instantiation of
// __leaf_sorter with the provided types. See [temp.inst]/1 of C++17 spec for the details.
static_assert(
sycl::is_device_copyable_v<oneapi::dpl::__par_backend_hetero::__leaf_sorter<noop_device_copyable,
Copy link
Contributor

@danhoeflinger danhoeflinger Nov 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this actually causes any problems for the compilation of the test, but we should probably create a utility range_device_copyable and a range_non_device_copyable which implement operator[], size(), begin(), end(), and provide a value and difference type in test/support/utils_device_copyable.h.
Use those types here instead of the noop functors.

Copy link
Contributor Author

@dmitriy-sobolev dmitriy-sobolev Nov 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It successfully compiles with the current approach. Let me investigate why, what I should have done initially.

If it is proven to be reliable, we can avoid adding extra code in the test. If I cannot prove it, I will reuse an existing range type (if possible), and add divice_copyable/non_device_copyable comparators (it is easier).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think its not fully instantiating the type in the compile time check, so it doesn't run into the issues in the type if it were created with those types. https://godbolt.org/z/d87x6eo1s

I'm a bit torn, as we could probably delete all but a simple device_copyable_type and non_device_copyable_type in our tests, but we wouldn't be checking types which are actually able to be instantiated.

I guess I'm OK with either direction we want to go. So far, we have created types to match, but wouldnt push hard against cleaning this up and relying only on a couple simple types if it works and compiles reliably for our testing purposes on our full compiler matrix.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for the example and description. I have a more tailored example here: https://godbolt.org/z/3WqGT68x6. Indeed, it does not instantiate the type entirely.

It seems there is no need for the compiler to instantiate the tested class entirely in that expression, so we can get away with noop classes. The standard (c++17) says the following about it:

Unless a class template specialization has been explicitly instantiated (17.8.2) or explicitly specialized (17.8.3),
the class template specialization is implicitly instantiated when the specialization is referenced in a context
that requires a completely-defined object type or when the completeness of the class type affects the semantics
of the program.

In my understanding the note implies that a class must be instantiated when it is needed in that particular context.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, thanks for finding the relevant section in the specification. I think technically we will generally be OK with just the simple noop classes.

I'm still undecided which I prefer really. I think the more realized types are a bit better to document what we are doing (I always look at tests documentation, at least in part). This is a big stretch here because we are really testing internals in an obscure and specific fashion. However, it may be more confusing to future readers with less realistic non-instantiable types.

I'm OK with merging as it is if that is what you would like to do. If we want to clean up the types to remove all but a pair of simple types in a separate PR, we can do that. We can probably add some comment explaining why this works and the goal of these specific tests, if it is not already clear.

noop_device_copyable>>,
"__leaf_sorter is not device copyable with device copyable types");

//__not_pred
static_assert(sycl::is_device_copyable_v<oneapi::dpl::__internal::__not_pred<noop_device_copyable>>,
"__not_pred is not device copyable with device copyable types");
Expand Down Expand Up @@ -423,6 +432,11 @@ test_non_device_copyable()
noop_non_device_copyable>>,
"__early_exit_find_or is device copyable with non device copyable types");

// __leaf_sorter
static_assert(!sycl::is_device_copyable_v<oneapi::dpl::__par_backend_hetero::__leaf_sorter<noop_device_copyable,
noop_non_device_copyable>>,
"__leaf_sorter is device copyable with non device copyable types");

//__not_pred
static_assert(!sycl::is_device_copyable_v<oneapi::dpl::__internal::__not_pred<noop_non_device_copyable>>,
"__not_pred is device copyable with non device copyable types");
Expand Down
Loading