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

RFC for zip_view implementation, for oneDPL C++20 #1931

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

RFC for zip_view implementation, for oneDPL C++20 #1931

Changes from 13 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
09bd199
[oneDPL][rfc][zip_view] + draft
MikeDvorskiy Nov 4, 2024
b56c860
Update README.md
MikeDvorskiy Nov 27, 2024
761ecb7
Update README.md
MikeDvorskiy Nov 29, 2024
7ef15f4
Update README.md
MikeDvorskiy Nov 29, 2024
e792b5b
Update README.md
MikeDvorskiy Nov 29, 2024
786d5c5
Update README.md
MikeDvorskiy Nov 29, 2024
171e5fe
Update README.md
MikeDvorskiy Nov 29, 2024
f9a2e45
[oneDPL][rfc][ranges][zip_view] + Implementation design proposal
MikeDvorskiy Dec 2, 2024
4a701fc
+ spelling fix
MikeDvorskiy Dec 3, 2024
cc3eac4
+ spelling fix
MikeDvorskiy Dec 3, 2024
52acfac
+Implementation proposal
MikeDvorskiy Dec 4, 2024
526a603
Delete rfcs/experimental/zip_view/README.md
MikeDvorskiy Dec 4, 2024
518aee0
Update README.md
MikeDvorskiy Dec 4, 2024
234dbdb
Update rfcs/proposed/zip_view/README.md
MikeDvorskiy Dec 17, 2024
c20ef9f
Update rfcs/proposed/zip_view/README.md
MikeDvorskiy Dec 17, 2024
aec760b
Update rfcs/proposed/zip_view/README.md
MikeDvorskiy Dec 17, 2024
9ab570b
Update rfcs/proposed/zip_view/README.md
MikeDvorskiy Dec 17, 2024
300e13c
Update rfcs/proposed/zip_view/README.md
MikeDvorskiy Dec 17, 2024
f51297e
Update rfcs/proposed/zip_view/README.md
MikeDvorskiy Dec 17, 2024
4cade93
Update rfcs/proposed/zip_view/README.md
MikeDvorskiy Dec 17, 2024
7b882fb
Update rfcs/proposed/zip_view/README.md
MikeDvorskiy Dec 17, 2024
2e9c98d
Update rfcs/proposed/zip_view/README.md
MikeDvorskiy Dec 17, 2024
214dcde
Update rfcs/proposed/zip_view/README.md
MikeDvorskiy Dec 18, 2024
1f7b132
Update README.md
MikeDvorskiy Dec 20, 2024
720f1d2
Update README.md
MikeDvorskiy Jan 15, 2025
416010b
Update README.md
MikeDvorskiy Jan 15, 2025
28aed7e
Update README.md
MikeDvorskiy Jan 15, 2025
1ce2407
Update README.md
MikeDvorskiy Jan 15, 2025
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
40 changes: 40 additions & 0 deletions rfcs/proposed/zip_view/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
# zip_view Support for the oneDPL Range APIs with C++20

## Introduction
`std::ranges::zip_view` is powerful utility enables developers to combine two or more ranges into a single view,
where each element is represented as a tuple containing corresponding elements from each input range.
MikeDvorskiy marked this conversation as resolved.
Show resolved Hide resolved

## Motivations
`std::ranges::zip_view` is a convenient way to combine multiple ranges into a single view, where each element of
the resulting range is a tuple containing one element from each of the input ranges. This can be particularly
useful for iterating over multiple collections in parallel. `std::ranges::zip_view` is introduced starting with C++23,
but there are many users who work with C++20 standard yet. So, oneDPL introduces `oneapi::dpl::ranges::zip_view`,
MikeDvorskiy marked this conversation as resolved.
Show resolved Hide resolved
which the same API and functionality as `std::ranges::zip_view`.
MikeDvorskiy marked this conversation as resolved.
Show resolved Hide resolved

### Key Requirements
akukanov marked this conversation as resolved.
Show resolved Hide resolved
`oneapi::dpl::ranges::zip_view` should be:
- compilable with C++20 version (minimum)
dmitriy-sobolev marked this conversation as resolved.
Show resolved Hide resolved
- API compliant with `std::ranges::zip_view`
MikeDvorskiy marked this conversation as resolved.
Show resolved Hide resolved
- random accessible view; the "underlying" views also should be random accessible
dmitriy-sobolev marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

This is how cppreference describes the category for std::ranges::zip_view:

zip_view always models input_range, and models forward_range, bidirectional_range, random_access_range, or sized_range if all adapted view types model the corresponding concept.

Do we want the same adaptive behavior, or do we want to require all the base ranges to be random access ranges?

Copy link
Contributor Author

@MikeDvorskiy MikeDvorskiy Dec 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In my understanding our intention was to provide zip_view like adaptor (for C++20), which "works (is pipeable)" with C++20 ranges/views and can be passed into oneDPL algorithms, because existing experimental::zip_view "doesn't work (is not pipeable)" with C++20 ranges/views.

As far as https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2024/p3179r2.html tells that only random access ranges are accepted for range based oneDPL algorithm, I think we want to support zip_view only for random_access_range - to require all the base ranges to be random access ranges.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, so there is no intent to make it adaptable to different range categories and usable more widely. It needs to be clear in the documentation & specification.

Copy link
Contributor

Choose a reason for hiding this comment

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

And we should also think of implications. If our zip_view cannot be used with ranges that are not random access, could it make developers hesitant to use it, possibly impacting oneDPL usage?

Copy link
Contributor Author

@MikeDvorskiy MikeDvorskiy Jan 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

In my understanding the proposed zip_view is just to use with oneDPL parallel algorithms, which accept just RA ranges. The goal was not adding zip_view for C++20 which works with any type ranges (like C++23 std::zip_view)
If developers don't hesitant to use oneDPL RA Range based parallel algorithms, the don't hesitant to use the proposed RA zip_view as well.

- in case of a device usage: a device copyable view itself and the "underlying" views also should be device copyable
Copy link
Contributor

Choose a reason for hiding this comment

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

The same question here: do we require base views to be device copyable, or do we say that the zip_view is device copyable if the base views are?

Copy link
Contributor Author

@MikeDvorskiy MikeDvorskiy Dec 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

yes, "a device copyable view itself" - is not clear and the sentence should be rephrased.

Yes, we say that the zip_view is device copyable if the base views are.
The following sentence guarantees that:
"To provide a device copyability requirement oneapi::dpl::__internal::tuple is proposed as tuple-like type underhood."

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. Please adjust the text to make the intent clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


`oneapi::dpl::ranges::zip_view::iterator` should be:
- value-swappable (https://en.cppreference.com/w/cpp/named_req/ValueSwappable)
- convertible to `oneapi::dpl::zip_iterator`
MikeDvorskiy marked this conversation as resolved.
Show resolved Hide resolved
- abble to use with the non-range algorithms
MikeDvorskiy marked this conversation as resolved.
Show resolved Hide resolved

### Implementation proposal
- `oneapi::dpl::ranges::zip_view` is designed as C++ class, which represents a range adaptor (see C++ Range Library).
MikeDvorskiy marked this conversation as resolved.
Show resolved Hide resolved
This class encapsulates a tuple-like type to keep a combination of two or more ranges.
- The implementation provides all necessary operators to satisfy 'random accessible view' requirement.
- To provide a device copyability requirement `oneapi::dpl::__internal::tuple` is proposed as tuple-like type underhood.
MikeDvorskiy marked this conversation as resolved.
Show resolved Hide resolved
- To provide a value-swappable requirement `oneapi::dpl::__internal::tuple` is proposed as a dereferenced value for
Copy link
Contributor

Choose a reason for hiding this comment

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

When it comes to the specification, this will need to be described as an unspecified tuple which satisfies some set of requirements, unless we want to make the move to specify our internal tuple and make it public (which I doubt we want to do).

Copy link
Contributor

Choose a reason for hiding this comment

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

In the context of this, I was looking at the specification for our current zip_iterator, and noticed a couple of discrepancies as compared to our implementation with regard to std::tuple vs our unspecified internal tuple type. I believe that our implementation is intentional and the "correct" thing here and the spec should be changed, because of issues with device copyability when composing types from zip_iterators. std::tuple is not trivially copyable but our internal tuple is.

uxlfoundation/oneAPI-spec#605

`oneapi::dpl::ranges::zip_view::iterator`, due to the standard `std::tuple` C++20 is not swappable type.
MikeDvorskiy marked this conversation as resolved.
Show resolved Hide resolved
- Usage of C++ concepts is desirable to write type requirements for types, methods and members of the class.
- C++20 is minimum supported version for the class. It allows to use modern C++ things like concepts and others.
MikeDvorskiy marked this conversation as resolved.
Show resolved Hide resolved

### Test coverage

- `oneapi::dpl::ranges::zip_view` is tested itself, base functionality.
Copy link
Contributor

Choose a reason for hiding this comment

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

What amount of testing would be necessary to ensure the API compliance with std::ranges::zip_view, stated as a requirement?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think to have at least one call of each method of oneapi::dpl::ranges::zip_view with result checking is quite enough.
As an additional test coverage we may consider to take LLVM tests for std::ranges::zip_view, if it is technically possible by C++23/C++20 reasons.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Talking about "the API compliance with std::ranges::zip_view". I mean the public methods and types are equivalent at least for underlying random sized ranges: ones are callable and result the same. And also pipeable feature is supported.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would we want a test which requires c++23 and uses std::ranges::zip_view itself as the ground truth here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would we want a test which requires c++23 and uses std::ranges::zip_view itself as the ground truth here?

I guess - No, we don't. We rely on std::ranges::zip_view is tested within C++ standard library good enough.
But we may add some test cases with oneDPL algo calls with std::ranges::zip_view C++23.

- should be tested with at least one oneDPL range based algorithm.
- should be tested with at least one oneDPL iterator based algorithm.
Loading