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

[oneDPL][ranges] + zip_view implementation for C++20 #1877

Open
wants to merge 31 commits into
base: main
Choose a base branch
from

Conversation

MikeDvorskiy
Copy link
Contributor

@MikeDvorskiy MikeDvorskiy commented Sep 25, 2024

[oneDPL][ranges] + zip_view implementation for C++20. The implementation and test.

  • namespace: oneapi::dpl::ranges
  • based on oneDPL device copyable tuple

@MikeDvorskiy MikeDvorskiy marked this pull request as draft September 25, 2024 15:06
@MikeDvorskiy MikeDvorskiy marked this pull request as ready for review September 27, 2024 14:04
@MikeDvorskiy MikeDvorskiy force-pushed the dev/mdvorski/zip_view branch 2 times, most recently from 603daed to 80ef9c5 Compare September 30, 2024 09:50
@danhoeflinger danhoeflinger self-requested a review October 2, 2024 12:21
Copy link
Contributor

@dmitriy-sobolev dmitriy-sobolev left a comment

Choose a reason for hiding this comment

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

Let me leave just a couple of minor comments. I am going to look at the whole PR during this week.

test/parallel_api/ranges/std_ranges_zip_view.pass.cpp Outdated Show resolved Hide resolved
test/parallel_api/ranges/std_ranges_zip_view.pass.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@dmitriy-sobolev dmitriy-sobolev left a comment

Choose a reason for hiding this comment

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

What do you think about testing the following properties of zip_view/zip?

  • begin + end (they should also probably be checked if they are iterators)
  • cbegin + cend
  • front + back
  • size
  • empty
  • operator bool
  • construction:
    • ranges::zip(...) + ranges::zip_view(...)
    • empty view (views::zip())
    • another view (also check if it does move construction)
    • another zip_view
  • is a customization point object

I assume that the functionality must match what is required from c++ standard looking at the description. That's why I suggest testing all these properties. Let me know if the implementation is expected to be more relaxed (and how if it is).

@dmitriy-sobolev
Copy link
Contributor

dmitriy-sobolev commented Oct 18, 2024

The description says:

... based on oneDPL device copyable tuple

According to SYCL 2020, std::tuple is also device copyable (if the wrapped types are also device copyable, of course). Does it make sense to use std::tuple instead of the internal one?

@danhoeflinger
Copy link
Contributor

danhoeflinger commented Oct 18, 2024

The description says:

... based on oneDPL device copyable tuple

According to SYCL 2020, std::tuple is also device copyable (if the wrapped types are also device copyable, of course). Does it make sense to use std::tuple instead of the internal one?

The advantage here of using oneDPL's tuple is that it is trivially copyable (for trivially copyable types) which means that any class or structure which uses oneDPL's tuple can be implicitly device copyable rather than requiring a specialization of sycl::is_device_copyable (because of the tuple). std::tuple isn't specified to be trivially copyable.

The advantage is not just to avoid extra code here but also to not impose requirements on users to provide these specializations for types which are composed of a zip_view as well.

There can be downsides to using oneDPLs tuple in that it may not be as fully featured as std::tuple in some ways but I think this is the better option.

@MikeDvorskiy
Copy link
Contributor Author

MikeDvorskiy commented Oct 18, 2024

to use std::tuple instead of the internal one?

Actually, before C++23 standard library there is a problem with std::tuple swap ability. It is bigger issue that doesn't allow to use std::tuple for zip_view implementation for oneDPL C++20.

include/oneapi/dpl/pstl/zip_view_impl.h Outdated Show resolved Hide resolved
include/oneapi/dpl/pstl/zip_view_impl.h Outdated Show resolved Hide resolved
include/oneapi/dpl/pstl/zip_view_impl.h Outdated Show resolved Hide resolved
include/oneapi/dpl/pstl/zip_view_impl.h Outdated Show resolved Hide resolved
static decltype(auto)
apply_to_tuple(_ReturnAdapter __tr, _F __f, _Tuple& __t)
{
return apply_to_tuple_impl(__tr, __f, __t, std::make_index_sequence<sizeof...(Views)>{});
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it makes sense to move functions to the apply_to_tuple_impl?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems I don't catch your question/suggestion..

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean modify to apply_to_tuple_impl(std::move(__tr), std::move(__f), __t, std::make_index_sequence)
Or we believe that these lambdas cannot capture a lot?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now I see what you mean..
The implementation uses the lambdas with no capture at all or one integer variable. So, since it is an 'internal' code we may pass further these lambdas by copy w/o any overheads.

include/oneapi/dpl/pstl/zip_view_impl.h Outdated Show resolved Hide resolved
include/oneapi/dpl/pstl/zip_view_impl.h Outdated Show resolved Hide resolved
include/oneapi/dpl/pstl/zip_view_impl.h Outdated Show resolved Hide resolved
include/oneapi/dpl/pstl/zip_view_impl.h Outdated Show resolved Hide resolved
include/oneapi/dpl/pstl/zip_view_impl.h Outdated Show resolved Hide resolved
@MikeDvorskiy MikeDvorskiy force-pushed the dev/mdvorski/zip_view branch 2 times, most recently from a9c5e7c to ad4e8ac Compare October 18, 2024 16:42
MikeDvorskiy and others added 17 commits October 22, 2024 14:31
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.

4 participants