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

MikeDvorskiy · 2024-09-25T15:05:58Z

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

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

dmitriy-sobolev

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

dmitriy-sobolev

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

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).

Update. I see that the PR integrates the whole LLVM test suite now. The table is not relevant now

dmitriy-sobolev · 2024-10-18T10:15:42Z

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 · 2024-10-18T13:20:00Z

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 · 2024-10-18T13:34:11Z

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

MikeDvorskiy · 2024-10-25T15:17:39Z

regarding > * cbegin + cend
cppreference:

cbegin
  
(C++23)
 
returns a constant iterator to the beginning of the range.
(public member function of std::ranges::view_interface<D>)
cend
  
(C++23)
 
returns a sentinel for the constant iterator of the range.
(public member function of std::ranges::view_interface<D>)

include/oneapi/dpl/pstl/zip_view_impl.h

kboyarinov · 2024-11-11T14:54:39Z

include/oneapi/dpl/pstl/zip_view_impl.h

+            if (x.current_ < y.current_)
+                return -1;
+            else if (x.current_ == y.current_)
+                return 0;
+            return 1; //x.current > y.current_


Why cannot we use x.current <=> y.current? Spaceship operator is undefined for some of our internal iterator types or it is undefined for internal tuple?

yes, spaceship operator is undefined for x.current_...

When adding comparison between internal tuples, we looked at adding spaceship and it was deemed not worth the effort at the time. We could use this as motivation for adding it, but it would require a bit of work. We would need to trigger it off of __cpp_lib_three_way_comparison >= 201907L, and in that case replace the existing comparison operators with the spaceship, to time it properly with std::tuple <=> operator

#1472 (comment)

The thought was that it wasn't worth the maintenance because we would need to implement both to support our full support matrix, and it would be only helpful for those explicitly calling <=> on the internal tuple (which is what we might want to do here as I understand it).

include/oneapi/dpl/pstl/zip_view_impl.h

dmitriy-sobolev · 2024-11-19T10:21:22Z

test/parallel_api/ranges/std_ranges_zip_view.pass.cpp

+
+    std::sort(zip_view_sort.begin(), zip_view_sort.begin() + max_n, [](const auto& val1, const auto& val2) { return std::get<0>(val1) > std::get<0>(val2); });
+    for(int i = 0; i < max_n; ++i)
+        assert(std::get<0>(zip_view_sort[i]) == max_n - 1 - i);


Our testing framework is not adapted for assert: it defines NDEBUG during release test builds. We should either avoid that definition, or do not rely on assert in the tests.

In spite of that we are using assert in our tests more then 10 files... It seems an issue should be created.

I know that a better way to use our own macro like EXPECT_TRUE f.e.

I will create it.

That sounds good. Could you do it? I see that there is still one assert in the test.

I will create it.

Done: #1945.

done.
And I suggest offline discussion about assert / EXPECT_TRUE / NDEBUG

test/parallel_api/ranges/std_ranges_zip_view.pass.cpp

dmitriy-sobolev · 2024-11-19T18:58:01Z

test/parallel_api/ranges/std_ranges_zip_view.pass.cpp

+    std::vector<int> vec1(max_n);
+    std::vector<int> vec2(max_n/2);
+
+    auto zip_view = dpl_ranges::zip(vec1, vec2);


Should zip also be checked for availability in oneapi::dpl::views namespace?

dmitriy-sobolev · 2024-11-20T10:04:38Z

regarding > * cbegin + cend cppreference:

cbegin
  
(C++23)
 
returns a constant iterator to the beginning of the range.
(public member function of std::ranges::view_interface<D>)
cend
  
(C++23)
 
returns a sentinel for the constant iterator of the range.
(public member function of std::ranges::view_interface<D>)

zip_view is based on view_iterface, and the later does not have cbegin/cend in c++20. That means our c++20 zip_view will not have cbegin/cend, and we are not going to provide them. Is that correct understanding?

kboyarinov · 2024-11-20T10:56:28Z

include/oneapi/dpl/pstl/zip_view_impl.h

+#ifndef _ONEDPL_ZIP_VIEW_IMPL_H
+#define _ONEDPL_ZIP_VIEW_IMPL_H
+
+#if _ONEDPL_CPP20_RANGES_PRESENT


Did not we think about having oneapi::dpl::zip_view as part of namespace std::ranges prior to C++23?
It will allow the user to have portable code for each version of the standard. Otherwise, we will need to think about aligning oneapi::dpl::zip_view with the C++23 and later as well.

Do you mean a kind of name injection into std::ranges prior to C++23? Good question.

On one hand - it is useful when a user is going to modify his code forward to C++23.
On the other hand, having std::ranges::zip_view (in result of name oneapi::dpl::zip_view injection), enforces us to be fully compliance with standard zip_view C++23. At least, the proposed zip_view implementation works only with random access views, for example. And we don't need more for oneDPL needs.

I would suggest rather to have an alias oneapi::dpl::zip_view, which is our own implementation class prior to C++23 and which is std::ranges::zip_view for C++23 and higher.

I like the idea of having the alias to std::ranges::zip_view in oneDPL for C++23 and higher. It perfectly fits the idea of having everything that should work in the offloaded code in oneapi::dpl namespace.

We can consider such an alias, but for me that would mean that our implementation of zip_view should support (almost) all of what the standard requires. And since zip_view is based on the standard tuple, which in C++20 has some limitations for our needs, I am not sure to which extent the compliance with the standard is feasible.

Also there still will be a question whether it should be a specified feature or an extension.

…n for oneDPL

…tructor

…ed for oneDPL

…ion for oneDPL

…for oneDPL

…or (from declare_iterator_category)

…ion for oneDPL

…&& _ONEDPL_GCC_VERSION >= 120100

… "1) Default constructor. Value-initializes all elements, if any"

…eDPL

kboyarinov · 2025-02-17T14:06:23Z

include/oneapi/dpl/pstl/tuple_impl.h

+    oneapi::dpl::__internal::__copy_assignable_holder<T1> holder{};
+    tuple<T...> next{};


I propose to add comment why {} are required since it is not trivial.

kboyarinov · 2025-02-17T14:19:43Z

include/oneapi/dpl/pstl/tuple_impl.h

+    template <typename U1, typename... U>
+    tuple&
+    operator=(const tuple<U1, U...>& other) const
+    {
+        holder.value = other.holder.value;
+        next = other.next;
+        return *this;
+    }


It makes sense when we assign constant tuple objects of non-constant references:
https://godbolt.org/z/TP8Gzhvxb (Failing with C++20, but works in C++23)

kboyarinov · 2025-02-17T14:21:49Z

test/parallel_api/ranges/range.zip/iterator/zip_view.arithmetic.pass.cpp

@@ -0,0 +1,151 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.


Should we include Intel Copyright as well?

kboyarinov · 2025-02-17T14:22:19Z

test/parallel_api/ranges/range.zip/iterator/zip_view.arithmetic.pass.cpp

+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17, c++20


Since these annotations are not required for our test system, I guess it can be removed from all tests

kboyarinov · 2025-02-17T14:23:44Z

test/parallel_api/ranges/range.zip/iterator/zip_view.arithmetic.pass.cpp

+template <typename _R>
+    concept __simple_view_concept =
+        std::ranges::view<_R> && std::ranges::range<const _R> && std::same_as<std::ranges::iterator_t<_R>,
+        std::ranges::iterator_t<const _R>> && std::same_as<std::ranges::sentinel_t<_R>, std::ranges::sentinel_t<const _R>>;


This concept is unused by this test. Why it is needed?

kboyarinov · 2025-02-17T14:40:16Z

include/oneapi/dpl/pstl/zip_view_impl.h

+        }
+
+      private:        
+        constexpr explicit iterator(current_type __t): current_(std::move(__t)) {}


Suggested change

constexpr explicit iterator(current_type __t): current_(std::move(__t)) {}

constexpr explicit iterator(current_type __t) : current_(std::move(__t)) {}

kboyarinov · 2025-02-17T14:41:01Z

include/oneapi/dpl/pstl/zip_view_impl.h

+
+  public:
+    zip_view() = default;
+    constexpr explicit zip_view(Views... views) : views_(std::move(views)...) {}


Should we apply code uglification everywhere for private APIs and arguments?

kboyarinov · 2025-02-17T14:47:41Z

include/oneapi/dpl/pstl/zip_view_impl.h

+                using return_tuple_type = reference_type;
+                return return_tuple_type(std::forward<decltype(__args)>(__args)...);


Suggested change

using return_tuple_type = reference_type;

return return_tuple_type(std::forward<decltype(__args)>(__args)...);

return reference_type(std::forward<decltype(__args)>(__args)...);

kboyarinov · 2025-02-17T14:52:11Z

include/oneapi/dpl/pstl/zip_view_impl.h

+            operator==(const iterator& x, const iterator& y) requires(
+                std::equality_comparable<std::ranges::iterator_t<__maybe_const<Const, Views>>>&&...)


Suggested change

operator==(const iterator& x, const iterator& y) requires(

std::equality_comparable<std::ranges::iterator_t<__maybe_const<Const, Views>>>&&...)

operator==(const iterator& x, const iterator& y)

requires(std::equality_comparable<std::ranges::iterator_t<__maybe_const<Const, Views>>>&&...)

Just formatting? or?

kboyarinov · 2025-02-17T14:54:33Z

include/oneapi/dpl/pstl/zip_view_impl.h

+        operator-(const iterator& x, const iterator& y) requires 
+        (std::sized_sentinel_for<std::ranges::iterator_t<__maybe_const<Const, Views>>, std::ranges::iterator_t<__maybe_const<Const, Views>>> && ...)


Suggested change

operator-(const iterator& x, const iterator& y) requires

(std::sized_sentinel_for<std::ranges::iterator_t<__maybe_const<Const, Views>>, std::ranges::iterator_t<__maybe_const<Const, Views>>> && ...)

operator-(const iterator& x, const iterator& y)

requires(std::sized_sentinel_for<std::ranges::iterator_t<__maybe_const<Const, Views>>,

std::ranges::iterator_t<__maybe_const<Const, Views>>> && ...)

Just formatting? or?

… iterators

… forward iterators" This reverts commit 6ff5b7a.

… oneDPL

… internal tuple

…uctor.

MikeDvorskiy marked this pull request as draft September 25, 2024 15:06

MikeDvorskiy requested review from rarutyun and kboyarinov September 25, 2024 15:10

MikeDvorskiy force-pushed the dev/mdvorski/zip_view branch from 9342293 to 91cf6b0 Compare September 26, 2024 12:19

MikeDvorskiy requested a review from akukanov September 26, 2024 13:39

MikeDvorskiy marked this pull request as ready for review September 27, 2024 14:04

MikeDvorskiy force-pushed the dev/mdvorski/zip_view branch 2 times, most recently from 603daed to 80ef9c5 Compare September 30, 2024 09:50

danhoeflinger self-requested a review October 2, 2024 12:21

dmitriy-sobolev reviewed Oct 7, 2024

View reviewed changes

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

dmitriy-sobolev reviewed Oct 11, 2024

View reviewed changes

kboyarinov reviewed Oct 18, 2024

View reviewed changes

MikeDvorskiy force-pushed the dev/mdvorski/zip_view branch 3 times, most recently from f1c08a6 to 74d52be Compare October 22, 2024 12:39

MikeDvorskiy requested review from kboyarinov and dmitriy-sobolev November 4, 2024 15:24

MikeDvorskiy force-pushed the dev/mdvorski/zip_view branch from 78abd7f to fdf3e9b Compare November 7, 2024 14:43

kboyarinov reviewed Nov 11, 2024

View reviewed changes

dmitriy-sobolev reviewed Nov 19, 2024

View reviewed changes

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

dmitriy-sobolev reviewed Nov 19, 2024

View reviewed changes

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

dmitriy-sobolev reviewed Nov 19, 2024

View reviewed changes

kboyarinov reviewed Nov 20, 2024

View reviewed changes

MikeDvorskiy force-pushed the dev/mdvorski/zip_view branch from 0e35be1 to 5f43b52 Compare November 28, 2024 17:45

MikeDvorskiy added 16 commits February 17, 2025 12:32

[oneDPL][ranges][zip_view][test] LLVM test arithmetict.pass adaptatio…

eb5f579

…n for oneDPL

[oneDPL][ranges][zip_view] + fixes in private zip_view::iterator cons…

d5f5616

…tructor

[oneDPL][ranges][zip_view][test] compare.pass test adaptation for oneDPL

9b0852d

[oneDPL][ranges][zip_view] + fixes in apply_to_tuple using

953eb3f

[oneDPL][ranges][zip_view][test] another LLVM test portion were adopt…

8e427fb

…ed for oneDPL

[oneDPL][ranges][zip_view] + iter_move friend method

0eb08b6

[oneDPL][ranges][zip_view][test] another couple of LLVM tests adaptat…

a766030

…ion for oneDPL

[oneDPL][ranges][zip_view] + iter_swap friend method

a2942ee

[oneDPL][ranges][zip_view][test] LLVM test iter_swap.pass adaptation …

3de9dd1

…for oneDPL

[oneDPL][ranges][zip_view] + fix: public inheritance for class iterat…

9f5f2d3

…or (from declare_iterator_category)

[oneDPL][ranges][zip_view][test] another LLVM test adaptation for oneDPL

35bc5dd

[oneDPL][ranges][zip_view][test] another couple of LLVM tests adaptat…

6923b5e

…ion for oneDPL

[oneDPL][ranges][zip_view][test] + test cases disabling #if __GNUC__ …

e4f2002

…&& _ONEDPL_GCC_VERSION >= 120100

[oneDPL][ranges][tuple] + a fix for default. According to std::tuple:…

612b5ae

… "1) Default constructor. Value-initializes all elements, if any"

[oneDPL][ranges][zip_view] + a fix: __maybe_const usage.

1e8dad9

[oneDPL][ranges][zip_view][test] another LLVM tests adaptation for on…

dce5f5e

…eDPL

MikeDvorskiy force-pushed the dev/mdvorski/zip_view branch from 4a6a1f5 to dce5f5e Compare February 17, 2025 11:32

MikeDvorskiy added 2 commits February 17, 2025 12:41

[oneDPL][ranges][zip_view][test] minor changes for begin.pass test

69771a3

[oneDPL][ranges][zip_view][test] the tests renamed: + prefix "zip_view."

175dffb

kboyarinov reviewed Feb 17, 2025

View reviewed changes

MikeDvorskiy added 3 commits February 17, 2025 19:12

[oneDPL] a fixe for __pattern_walk2 implementation in case of forward…

6ff5b7a

… iterators

[oneDPL][ranges][zip_view] removed spaces

7381003

Revert "[oneDPL] a fixe for __pattern_walk2 implementation in case of…

5ae1d7c

… forward iterators" This reverts commit 6ff5b7a.

MikeDvorskiy force-pushed the dev/mdvorski/zip_view branch from 3cc0ea8 to 8c17b90 Compare February 19, 2025 10:35

MikeDvorskiy added 3 commits February 19, 2025 11:56

[oneDPL][ranges][zip_view][test] additional LLVM tests adaptation for…

1b16802

… oneDPL

[oneDPL][ranges][tuple] + a fix for Value-initializes all elements of…

6e774ca

… internal tuple

[oneDPL][ranges][zip_view][test] + #include "support/test_config.h"

c2156af

MikeDvorskiy force-pushed the dev/mdvorski/zip_view branch from 8c17b90 to c2156af Compare February 19, 2025 10:56

MikeDvorskiy added 2 commits February 19, 2025 17:24

[oneDPL][ranges][tuple] + a fix for the internal tuple default constr…

11c8521

…uctor.

[oneDPL][ranges][zip_view] + namespace views = ranges::views;

8894166

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

MikeDvorskiy commented Sep 25, 2024 •

edited

Loading

dmitriy-sobolev left a comment

dmitriy-sobolev left a comment •

edited

Loading

dmitriy-sobolev commented Oct 18, 2024 •

edited

Loading

danhoeflinger commented Oct 18, 2024 •

edited

Loading

MikeDvorskiy commented Oct 18, 2024 •

edited

Loading

MikeDvorskiy commented Oct 25, 2024 •

edited

Loading

kboyarinov Nov 11, 2024

MikeDvorskiy Nov 14, 2024

danhoeflinger Nov 14, 2024

danhoeflinger Nov 14, 2024

dmitriy-sobolev Nov 19, 2024

MikeDvorskiy Nov 19, 2024 •

edited

Loading

dmitriy-sobolev Nov 19, 2024

dmitriy-sobolev Nov 21, 2024

MikeDvorskiy Dec 5, 2024

dmitriy-sobolev Nov 19, 2024 •

edited

Loading

dmitriy-sobolev commented Nov 20, 2024 •

edited

Loading

kboyarinov Nov 20, 2024

MikeDvorskiy Dec 3, 2024 •

edited

Loading

kboyarinov Dec 4, 2024

akukanov Feb 12, 2025 •

edited

Loading

kboyarinov Feb 17, 2025

kboyarinov Feb 17, 2025

kboyarinov Feb 17, 2025

kboyarinov Feb 17, 2025

kboyarinov Feb 17, 2025

kboyarinov Feb 17, 2025

kboyarinov Feb 17, 2025

kboyarinov Feb 17, 2025

kboyarinov Feb 17, 2025

MikeDvorskiy Feb 18, 2025

kboyarinov Feb 17, 2025

MikeDvorskiy Feb 18, 2025

		oneapi::dpl::__internal::__copy_assignable_holder<T1> holder{};
		tuple<T...> next{};

	constexpr explicit iterator(current_type __t): current_(std::move(__t)) {}
	constexpr explicit iterator(current_type __t) : current_(std::move(__t)) {}

		using return_tuple_type = reference_type;
		return return_tuple_type(std::forward<decltype(__args)>(__args)...);

	using return_tuple_type = reference_type;
	return return_tuple_type(std::forward<decltype(__args)>(__args)...);
	return reference_type(std::forward<decltype(__args)>(__args)...);

		operator==(const iterator& x, const iterator& y) requires(
		std::equality_comparable<std::ranges::iterator_t<__maybe_const<Const, Views>>>&&...)

		operator-(const iterator& x, const iterator& y) requires
		(std::sized_sentinel_for<std::ranges::iterator_t<__maybe_const<Const, Views>>, std::ranges::iterator_t<__maybe_const<Const, Views>>> && ...)

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

Are you sure you want to change the base?

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

Conversation

MikeDvorskiy commented Sep 25, 2024 • edited Loading

dmitriy-sobolev left a comment

Choose a reason for hiding this comment

dmitriy-sobolev left a comment • edited Loading

Choose a reason for hiding this comment

dmitriy-sobolev commented Oct 18, 2024 • edited Loading

danhoeflinger commented Oct 18, 2024 • edited Loading

MikeDvorskiy commented Oct 18, 2024 • edited Loading

MikeDvorskiy commented Oct 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeDvorskiy Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitriy-sobolev Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

dmitriy-sobolev commented Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

MikeDvorskiy Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akukanov Feb 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeDvorskiy commented Sep 25, 2024 •

edited

Loading

dmitriy-sobolev left a comment •

edited

Loading

dmitriy-sobolev commented Oct 18, 2024 •

edited

Loading

danhoeflinger commented Oct 18, 2024 •

edited

Loading

MikeDvorskiy commented Oct 18, 2024 •

edited

Loading

MikeDvorskiy commented Oct 25, 2024 •

edited

Loading

MikeDvorskiy Nov 19, 2024 •

edited

Loading

dmitriy-sobolev Nov 19, 2024 •

edited

Loading

dmitriy-sobolev commented Nov 20, 2024 •

edited

Loading

MikeDvorskiy Dec 3, 2024 •

edited

Loading

akukanov Feb 12, 2025 •

edited

Loading