[onedpl][ranges] L1 Range based algo API and implementation #2272

MikeDvorskiy · 2025-05-20T12:52:40Z

[onedpl][ranges] L1 Range based algo API and implementation, according to RFC proposal #2269

reverse
reverse_copy; (return values semantic might be changed in the future or this algo will be removed)
unique
unique_copy
swap_ranges (extra algo, also usefull)

dmitriy-sobolev

I've checked the signatures for their compliance with https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2025/p3179r8.html#modify_copy - everything looks good.

include/oneapi/dpl/pstl/glue_algorithm_ranges_impl.h

dmitriy-sobolev · 2025-05-26T12:36:04Z

include/oneapi/dpl/pstl/glue_algorithm_ranges_impl.h

+        return oneapi::dpl::ranges::copy(std::forward<_ExecutionPolicy>(__exec), std::ranges::reverse_view(__in_r),
+            std::forward<_OutRange>(__out_r));


__pattern_reverse_copy from the device backend also uses an optimized __reverse_copy routine. Let's reuse it.

yes, it makes sense for hetero policy.

Lets wait an update from C++ committee. There is a chance that ranges::reverse_copy with policy will be removed from the parallel algo set.

dmitriy-sobolev · 2025-05-27T13:34:14Z

include/oneapi/dpl/pstl/glue_algorithm_ranges_impl.h

+
+namespace __internal
+{
+struct __reverse_copy_fn


There is no a corresponding test. Could you add it?

Lets wait an update from C++ committee. There is a chance that ranges::reverse_copy with policy will be removed from the parallel algo set.

include/oneapi/dpl/pstl/hetero/algorithm_ranges_impl_hetero.h

SergeyKopienko

In this changes I see more then one new lambdas like

template <typename _Tag, typename _ExecutionPolicy, typename _R, typename _Comp, typename _Proj>
std::ranges::borrowed_subrange_t<_R>
__pattern_unique(_Tag __tag, _ExecutionPolicy&& __exec, _R&& __r, _Comp __comp, _Proj __proj)
{
    auto __pred_2 = [__comp, __proj](auto&& __val1, auto&& __val2) { return std::invoke(__comp, std::invoke(__proj,
        std::forward<decltype(__val1)>(__val1)), std::invoke(__proj, std::forward<decltype(__val2)>(__val2)));};

    auto __beg = std::ranges::begin(__r);
    auto __end = __beg + std::ranges::size(__r);
    auto __it = oneapi::dpl::__internal::__pattern_unique(__tag, std::forward<_ExecutionPolicy>(__exec),
                                                          __beg, __end, __pred_2);

    return std::ranges::borrowed_subrange_t<_R>(__it, __end);
}

ehich created in some template code, parametriszed by ExecutionPolicy as template parameter.

This mean you again introduce the problem with ExecutionPolicy passed as l-value or r-value.

If all lambda's like that created only in host code. please insert

static_assert(__is_serial_tag_v<_Tag> || __is_parallel_forward_tag_v<_Tag>);

in these functions.

SergeyKopienko · 2025-05-27T15:26:44Z

include/oneapi/dpl/pstl/utils.h

    void
-    operator()(_ReferenceType1 __x, _ReferenceType2 __y) const
+    operator()(_Type1&& __x, _Type2&& __y) const


I believe this change looks not quite correct.
Because now you are using here universal reference : this mean you may pass into this function const reference too.
But it's invalid use case and I believe better to rewrite this like

operator()(_Type1& __x, _Type2& __y) const

The second moment - now this function ready to swap anything.
So you change semantic, because before this change struct __swap_fn require types in template parameters.
Are you sure that this change is correct?

But it's invalid use case and I believe better to rewrite this like

No.... For example, a proxy type, like tuple<int&, int&> doesn't match to _Type1& __x.

So, before swap_range algo did not work with zip_view and zip_iterator.... and other data where dereferenced value is a rvalue (a proxy object).
So, the change fixes such cases.
And yes, it makes sense to add such test cases - oneDPl doesn't have ones.

SergeyKopienko · 2025-05-27T15:31:09Z

Minor comment: not all :: before std in changed lines has been removed.

test/parallel_api/ranges/std_ranges_swap.pass.cpp

test/parallel_api/ranges/std_ranges_unique.pass.cpp

SergeyKopienko · 2025-05-27T15:55:28Z

@MikeDvorskiy could you you please check and fix in this PR the issue #2283 ?
Thanks.

dmitriy-sobolev · 2025-05-27T16:38:00Z

test/parallel_api/ranges/std_ranges_reverse.pass.cpp

+    //A checker below modifies a return type; a range based version with policy has another return type.
+    auto reverse_checker = [](auto&&... args) {
+        return std::ranges::reverse(std::forward<decltype(args)>(args)...);
+    };


I do not quite understand why and how the return type is changed.

std::ranges::reverse, when fed with a range, returns ranges::borrowed_iterator_t<R>. The same type is returned by the algorithm with the policy.

Is that checker really needed?

I guess, the comment is not relevant.

I suppose that the checker can be removed as well.

dmitriy-sobolev · 2025-05-27T17:07:59Z

test/parallel_api/ranges/std_ranges_swap.pass.cpp

+        return ret_type{std::ranges::begin(r1) + size, std::ranges::begin(r2) + size};
+    };
+
+    test_range_algo<0, int, data_in_out_lim>{big_sz}(dpl_ranges::swap_ranges,  swap_ranges_checker);


data_in_out_lim checks only "out" range. I expect that a test for swap_ranges should check both.

No, data_in_out_lim checks "out" range and :"in' range. Please take a look at the code:

//test case size of input range is less than size of output and vice-versa process_data_in_out(max_n, r_size/2, r_size, exec, algo, checker, args...); process_data_in_out(max_n, r_size, r_size/2, exec, algo, checker, args...);

These commands run the test with sequences with different sizes:

oneDPL/test/parallel_api/ranges/std_ranges_test.h

Lines 279 to 280 in c57830c

process_data_in_out(max_n, r_size/2, r_size, exec, algo, checker, args...);

process_data_in_out(max_n, r_size, r_size/2, exec, algo, checker, args...);

I meant the check of the data inside of the sequences. I've found only one such a check, which is performed over the second sequence:

oneDPL/test/parallel_api/ranges/std_ranges_test.h

Line 256 in c57830c

EXPECT_EQ_N(cont_exp().begin(), cont_out().begin(), n, (std::string("wrong effect algo with ranges: ") + typeid(Algo).name()).c_str());

The data in the first sequence is not checked.

test/parallel_api/ranges/std_ranges_unique.pass.cpp

MikeDvorskiy · 2025-05-30T15:45:14Z

This mean you again introduce the problem with ExecutionPolicy passed as l-value or r-value.

Essentially, no. This is a host part of the __pattern_unique implementation.
But yes, we can use oneapi::dpl::__internal::__compare functor instead of introducing new lambda.

…vel API

…l API

…et_intersection high level API

…ric_difference, high level API

…her PR

…ernal::__swap_fn

…Tag> || typename _Tag::__is_vector{}); and usage of some predefined functors

MikeDvorskiy requested review from akukanov, rarutyun and dmitriy-sobolev May 20, 2025 12:52

MikeDvorskiy marked this pull request as draft May 20, 2025 12:52

MikeDvorskiy force-pushed the dev/mdvorski/ranges_algo_L1-origin_UXL branch 8 times, most recently from 6ef1d07 to 3b9fa92 Compare May 20, 2025 14:03

MikeDvorskiy marked this pull request as ready for review May 21, 2025 15:45

MikeDvorskiy force-pushed the dev/mdvorski/ranges_algo_L1-origin_UXL branch 2 times, most recently from b02531f to 25b1761 Compare May 23, 2025 15:53

dmitriy-sobolev reviewed May 26, 2025

View reviewed changes

dmitriy-sobolev reviewed May 27, 2025

View reviewed changes

MikeDvorskiy force-pushed the dev/mdvorski/ranges_algo_L1-origin_UXL branch from a53327b to c0d749c Compare May 27, 2025 13:53

dmitriy-sobolev reviewed May 27, 2025

View reviewed changes

include/oneapi/dpl/pstl/hetero/algorithm_ranges_impl_hetero.h Outdated Show resolved Hide resolved

include/oneapi/dpl/pstl/hetero/algorithm_ranges_impl_hetero.h Outdated Show resolved Hide resolved

SergeyKopienko requested changes May 27, 2025

View reviewed changes

SergeyKopienko reviewed May 27, 2025

View reviewed changes

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

SergeyKopienko reviewed May 27, 2025

View reviewed changes

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

dmitriy-sobolev reviewed May 27, 2025

View reviewed changes

test/parallel_api/ranges/std_ranges_unique.pass.cpp Show resolved Hide resolved

MikeDvorskiy force-pushed the dev/mdvorski/ranges_algo_L1-origin_UXL branch 2 times, most recently from 22e0a8c to 5987bc0 Compare May 30, 2025 14:24

MikeDvorskiy force-pushed the dev/mdvorski/ranges_algo_L1-origin_UXL branch 2 times, most recently from ea3d9de to aa0ae5b Compare May 30, 2025 15:42

MikeDvorskiy force-pushed the dev/mdvorski/ranges_algo_L1-origin_UXL branch 3 times, most recently from 299a3cc to db5bd13 Compare June 5, 2025 15:35

MikeDvorskiy force-pushed the dev/mdvorski/ranges_algo_L1-origin_UXL branch from db5bd13 to 6cc066b Compare June 10, 2025 14:13

MikeDvorskiy added 22 commits June 12, 2025 12:26

[oneDPL][ranges][L1] + ranges::reverse, ranges::reverse_copy, high le…

d30bb56

…vel API

[oneDPL][ranges][L1] + ranges::unique, ranges::unique_copy, high leve…

ac103a1

…l API

[oneDPL][ranges][L1] + ranges::includes, ranges::set_union, ranges::s…

c3f437e

…et_intersection high level API

[oneDPL][ranges][L1] + ranges::set_difference, ranges::set_set_symmet…

42a98e8

…ric_difference, high level API

[oneDPL][ranges][L1] + __pattern_unique impl

d4048e2

[oneDPL][ranges][L1] + usage of oneapi::dpl::__internal::__compare

a3dc444

[oneDPL][ranges][L1] + __pattern_unique_copy impl

ce723c2

[oneDPL][ranges][L1] + ranges::swap_ranges, __ranges::pattern_swap

8e8a1c5

[oneDPL][ranges][L1] removed set algo range api; it is moving to anot…

683585e

…her PR

[oneDPL][ranges][L1] + typo fixes

4aaa892

[oneDPL][ranges][L1] + typo fixes

dfee286

[oneDPL][ranges][L1] + typo fixes

786044c

[oneDPL][ranges][L1] + fixe; __pattern_swap_ranges

31a659c

[oneDPL][ranges][L1] + a trivial pre-check for pattern_swap

6fdd2cb

[oneDPL][ranges][L1][tests] + a test for ranges::swap_ranges

72b1f8e

[oneDPL][ranges][L1][tests] + a test for ranges::reverse

dab8e9a

[oneDPL][ranges][L1][tests] + a test for ranges::unique

4f8f8f5

[oneDPL][ranges][L1][tests] + a test for ranges::unique_copy

81fd65a

[oneDPL][ranges][L1] removed #include "utils.h" // oneapi::dpl::__int…

b725006

…ernal::__swap_fn

[oneDPL][ranges][L1] + ranges __pattern_reverse

e7b7b63

[oneDPL][ranges][L1] + review minor comments addressed.

0740bda

[oneDPL][ranges][L1] host part, + static_assert(__is_parallel_tag_v<_…

0c2f14c

…Tag> || typename _Tag::__is_vector{}); and usage of some predefined functors

MikeDvorskiy force-pushed the dev/mdvorski/ranges_algo_L1-origin_UXL branch from 6cc066b to 0c2f14c Compare June 12, 2025 10:26

		return oneapi::dpl::ranges::copy(std::forward<_ExecutionPolicy>(__exec), std::ranges::reverse_view(__in_r),
		std::forward<_OutRange>(__out_r));

	process_data_in_out(max_n, r_size/2, r_size, exec, algo, checker, args...);
	process_data_in_out(max_n, r_size, r_size/2, exec, algo, checker, args...);

[onedpl][ranges] L1 Range based algo API and implementation #2272

Are you sure you want to change the base?

[onedpl][ranges] L1 Range based algo API and implementation #2272

Uh oh!

Conversation

MikeDvorskiy commented May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmitriy-sobolev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitriy-sobolev May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

SergeyKopienko left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SergeyKopienko commented May 27, 2025

Uh oh!

Uh oh!

Uh oh!

SergeyKopienko commented May 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitriy-sobolev May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitriy-sobolev Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MikeDvorskiy commented May 30, 2025

Uh oh!

Uh oh!

MikeDvorskiy commented May 20, 2025 •

edited

Loading

dmitriy-sobolev May 27, 2025 •

edited

Loading

SergeyKopienko left a comment •

edited

Loading

dmitriy-sobolev May 27, 2025 •

edited

Loading

dmitriy-sobolev Jun 9, 2025 •

edited

Loading