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

Cxx20standard #1768

Draft
wants to merge 18 commits into
base: distributed-ranges
Choose a base branch
from

Conversation

lslusarczyk
Copy link
Contributor

@lslusarczyk lslusarczyk commented Aug 6, 2024

CXX-20 lacks zip_view (used one from oneDPL), enumerate (implemented self simple version).
Anyway still there are some problems and I think I can no longer hold everything else to work on this further.

Any help on this is appreciated.

Currently fails with

/tmp/oneDPL/include/oneapi/dpl/internal/distributed_ranges_impl/sp/algorithms/../../detail/segments_tools.hpp:59:37:
 error: invalid operands to binary expression ('decltype(zip_view<iota_view<unsigned long, unsigned long>, vector<remote_vector<int, device_allocator<int, 0>>, allocator<remote_vector<int, device_allocator<int, 0>>>> &>(
::std::forward<std::ranges::iota_view<unsigned long, unsigned long>>(args), ::
std::forward<std::vector<oneapi::dpl::experimental::dr::sp::remote_vector<int, oneapi::dpl::experimental::dr::sp::device_allocator<int>>> &>(args)))'
 (aka 'zip_view<std::ranges::iota_view<unsigned long, unsigned long>, std::vector<oneapi::dpl::experimental::dr::sp::remote_vector<int, oneapi::dpl::experimental::dr::sp::device_allocator<int, 0>>, std::allocator<oneapi::dpl::experimental::dr::sp::remote_vector<int, oneapi::dpl::experimental::dr::sp::device_allocator<int, 0>>>> &>')
 and '_Partial<_Take, decay_t<unsigned long>>' 
(aka '_Partial<std::ranges::views::_Take, unsigned long>'))
   59 |     return make_enumerate(segments) | stdrng::views::take(last_seg + 1) |

@lslusarczyk
Copy link
Contributor Author

@akukanov , @MikeDvorskiy , here is my work in progress regarding making DR working C++-20 standard.
Fixing one problem reveals a new ones. I don't know when it will end, but if you think continuing this work is worth our effort, then we may continue and you're welcome to contribute.

@lslusarczyk
Copy link
Contributor Author

... maybe alternative is to officially say users to use ranges-v3 with oneDPL::DR if they need lower than 23 standard. This should work out of the box.

@@ -112,7 +112,7 @@ sort(R&& r, Compare comp = Compare())

T* medians = sycl::malloc_device<T>(n_segments * n_splitters, sp::devices()[0], sp::context());

for (auto&& [segment_id_, segment] : stdrng::views::enumerate(segments))
for (auto&& [segment_id_, segment] : dr::__detail::make_enumerate(segments))
Copy link
Contributor

@MikeDvorskiy MikeDvorskiy Aug 6, 2024

Choose a reason for hiding this comment

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

It turns out that we are using a functional approach for the sake of a functional approach...
I would suggest a little bit re-writing this loop is following way (w/o make_enumerate):

for (auto segment_id = 0, n =  stdrng::size(segment); segment_id < n; ++segment_id)
{
         auto segment = segments[segment_id ];
.......

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MikeDvorskiy , thanks for looking into this. Looks acceptable, although it is more lines of code.

Are other places with enumarate also easily changeable? Do you think it is more clean than having make_enumerate' or does it fixes some problems which make_enumerate` has (if it has)?

Enumerate is quite common programming pattern making code shorter and more readable.

I don't object of changing all places where there is enumerate to some code like this if it is needed to add C++20 support. Please feel free to clone my change to your branch and continue adding any changes you think may be good for C++20 support.

@akukanov
Copy link
Contributor

akukanov commented Aug 7, 2024

I have talked to Mikhail. It sounds like our internal zip_view has some built-in assumptions that complicate its use as a replacement for the standard one.

Let's keep this patch as a reference - as I still think there should be our own C++20 zip_view for both "regular" and distributed range algorithms, - but I am fine to postpone resolution of this question for a later update.

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.

3 participants