[onedpl][ranges] L3 Range based algo API and implementation (memory group) #2292

MikeDvorskiy · 2025-06-02T15:41:08Z

[onedpl][ranges] L3 Range based algo API and implementation (memory group)

test/parallel_api/ranges/std_ranges_memory_test.h

SergeyKopienko · 2025-06-11T13:32:51Z

test/parallel_api/ranges/std_ranges_memory.pass.cpp

+    int val1; //value initialization
+    int val2;
+
+    Elem_0(): val1() {}


Can you explain in comments the idea of works with structure in which data is uninitialized and from this point may have any state?

SergeyKopienko · 2025-06-11T13:36:00Z

include/oneapi/dpl/pstl/glue_memory_ranges_impl.h

+    std::ranges::borrowed_iterator_t<_R>
+    operator()(_ExecutionPolicy&& __exec, _R&& __r) const
+    {
+        const auto __dispatch_tag = oneapi::dpl::__ranges::__select_backend(__exec);


In another places I see the code like

const auto __dispatch_tag = oneapi::dpl::__ranges::__select_backend(__exec, __rng);

where we pass some __rng into __select_backend call.

From what it depends?

good catch, thanks!

There are more than one place like that, not all from this PR.

Update: Actually, the original comment is not valid here.
"another places" - you mind the experimental code... In the product code we pass only policy into select_backend function.
In "product" code we have random access ranges checks via concepts in the template description. So, we dont need do it the second time by passing ranges into the select_backend function.

Probably this mean we should move __select_backend function from the call

const auto __dispatch_tag = oneapi::dpl::__ranges::__select_backend(__exec, __rng);

into experimental namespace too?

I don't see a reason for that...

So I see on our code two funtions:

namespace oneapi { namespace dpl { namespace __ranges { template <typename _KernelName, typename... _Ranges> oneapi::dpl::__internal::__hetero_tag<oneapi::dpl::__internal::__device_backend_tag> __select_backend(const execution::device_policy<_KernelName>&, _Ranges&&...) { return {}; } #if _ONEDPL_FPGA_DEVICE template <unsigned int _Factor, typename _KernelName, typename... _Ranges> oneapi::dpl::__internal::__hetero_tag<oneapi::dpl::__internal::__fpga_backend_tag> __select_backend(const execution::fpga_policy<_Factor, _KernelName>&, _Ranges&&...) { return {}; } #endif } // namespace __ranges } // namespace dpl } // namespace oneapi

In the calls from experimental code you pass ranges into it, from non-experimental code you don't pass ranges.
But in any case ranges never used inside the __select_backend function, as we may see.
Can you explain the overall idea of that?

Related discussion: #1457 (comment). Mikhail's conclusion was to remove __select_backend accepting ranges when we remove the experimental range algorithms.

However, moving it into the experimental namespace looks reasonable to disambiguate different __select_backend but not critical in this PR.

Anyway, I suggest creating an issue to remove/refactor __select_backend with ranges in order not to forget about it. or doing that in a separate PR.

SergeyKopienko · 2025-06-11T13:37:56Z

include/oneapi/dpl/pstl/memory_ranges_impl.h

+
+    oneapi::dpl::uninitialized_fill(std::forward<_ExecutionPolicy>(__exec), __first, __last, __value);
+
+    return {__last};


Suggested change

return {__last};

return __last;

{} is written intentionally, because it is equivalent to std::ranges::borrowed_iterator_t<_R>{__last} (see the return type for the function).

Will we have compile error in the next case?

return __last;

Just for note: practically we never have returns with this form for cases when we return one value, as I seen.

return __last; tells the function returns an instance of "decltype(__last)"

return {__last}; tells the function returns an instance of new type (return type), which is created from __last.

My two cents:

I like (1) when a function is short: no duplication, you see the return type and {} suggests that is it not the type of __last.

I do not like (2). It is as (1) but it does not benefit from {}. Adding {} is not that extra number of symbol we should avoid.

I do not like (3), because I think that having the return type in the function signature is more important.

I like (4) when a function is large.

It's mostly a matter of preference (and now looks like bikeshedding), but __pattern_uninitialized_fill is small, so I vote for (1).

Probably it's a good topic to discuss shortly on our meeting.

UPD: the option (1) described at https://en.cppreference.com/w/cpp/language/list_initialization.html
And this variant has strongly defined semantic when make sense to apply in.
As far as we haven't compile errors here there is no reasons to apply it in the discussing cases I believe.

I agree here fully with @dmitriy-sobolev . (1) when short, (4) when large.

I think these options make the code most clear and readable, which is my rule of thumb.

After some discussion with @SergeyKopienko offline and exploring with godbolt, I realized that (1) is not explicitly invoking the constructor, but rather creating an initializer list, and then still using implicit construction of the return type from that initializer list.

https://godbolt.org/z/dTo84fooz

While not the exact same error message, (1) and (2) both don't work with explicit constructors. I guess (1) may give the perception that the type will be changing, (1) is not a shorthand equivalent to (4).

So, I guess perhaps I prefer (4) generally. For short functions, I think (1) and (2) are both fine, its probably not worth a whole lot more time discussing.

...from cppreference.com :

https://en.cppreference.com/w/cpp/language/return.html

2) Uses [copy-list-initialization](https://en.cppreference.com/w/cpp/language/list_initialization.html) to construct the return value of the function.

https://en.cppreference.com/w/cpp/language/list_initialization.html

8) in a return statement with a brace-enclosed initializer list used as the return expression and list-initialization initializes the returned object

SergeyKopienko · 2025-06-11T13:38:23Z

include/oneapi/dpl/pstl/memory_ranges_impl.h

+
+    oneapi::dpl::destroy(std::forward<_ExecutionPolicy>(__exec), __first, __last);
+
+    return {__last};


Suggested change

return {__last};

return __last;

See my explanation above.

…vel API

…l API

…et_intersection high level API

…ric_difference, high level API

…_tag_v<_Tag> || typename _Tag::__is_vector{}); and usage of some predefined functors" This reverts commit 0c2f14c.

This reverts commit 0740bda.

This reverts commit e7b7b63.

…l::__internal::__swap_fn" This reverts commit b725006.

This reverts commit 81fd65a.

This reverts commit 4f8f8f5.

This reverts commit dab8e9a.

This reverts commit 72b1f8e.

This reverts commit 6fdd2cb.

This reverts commit 31a659c.

This reverts commit 786044c.

This reverts commit dfee286.

This reverts commit 4aaa892.

… to another PR" This reverts commit 683585e.

…_swap" This reverts commit 8e8a1c5.

This reverts commit ce723c2.

…mpare" This reverts commit a3dc444.

This reverts commit d4048e2.

…t_symmetric_difference, high level API" This reverts commit 42a98e8.

…anges::set_intersection high level API" This reverts commit c3f437e.

…igh level API" This reverts commit ac103a1.

… high level API" This reverts commit d30bb56.

SergeyKopienko · 2025-06-12T11:00:54Z

test/parallel_api/ranges/std_ranges_memory.pass.cpp

+    int val1;
+    int val2;
+
+    Elem() { val1 = 1; }


I think we should have additional comments in the code why we don't initialize each field in constructors.

SergeyKopienko · 2025-06-12T11:00:59Z

test/parallel_api/ranges/std_ranges_memory.pass.cpp

+    int val1;
+    int val2;
+
+    Elem_0(): val1() {} //val1 has a value initialization here


I think we should have additional comments in the code why we don't initialize each field in constructors.

dmitriy-sobolev · 2025-06-16T08:50:17Z

include/oneapi/dpl/pstl/glue_memory_ranges_impl.h

+template<typename _R>
+concept nothrow_random_access_range =
+    std::ranges::range<_R> && nothrow_random_access_iterator<std::ranges::iterator_t<_R>> &&
+    nothrow_sentinel_for<std::ranges::sentinel_t<_R>, std::ranges::iterator_t<_R>>;


The corresponding concepts in the c++ standard are for exposition only. Should we move these concepts into an internal namespace then?

dmitriy-sobolev · 2025-06-16T09:25:33Z

include/oneapi/dpl/pstl/glue_memory_ranges_impl.h

+
+struct __uninitialized_move_fn
+{
+    template<typename _ExecutionPolicy, oneapi::dpl::ranges::nothrow_random_access_range _InRange,


Only the output range is required to be nothrow according to the proposal.
The same applies to uninitialized_copy above.

dmitriy-sobolev · 2025-06-16T09:36:00Z

include/oneapi/dpl/pstl/glue_memory_ranges_impl.h

+{
+    template<typename _ExecutionPolicy, oneapi::dpl::ranges::nothrow_random_access_range _InRange,
+             oneapi::dpl::ranges::nothrow_random_access_range _OutRange>
+    requires std::constructible_from<std::ranges::range_value_t<_InRange>, std::ranges::range_reference_t<_OutRange>>


It seems that the order is confused.

Suggested change

requires std::constructible_from<std::ranges::range_value_t<_InRange>, std::ranges::range_reference_t<_OutRange>>

requires std::constructible_from<std::ranges::range_value_t<_OutRange>, std::ranges::range_reference_t<_InRange>>

The same applies to uninitialized_move below.

dmitriy-sobolev · 2025-06-16T09:53:26Z

include/oneapi/dpl/pstl/glue_memory_ranges_impl.h

+template<typename _S, typename _I>
+concept nothrow_sentinel_for = std::sentinel_for<_S, _I>;


It defacto imposes no restrictions on exceptions, it's just an alias. What is the purpose of that? Should it be implemented this way?

dmitriy-sobolev · 2025-06-16T12:25:26Z

test/parallel_api/ranges/std_ranges_memory_test.h

+        run_one_policy(alloc, oneapi::dpl::execution::seq, algo, checker, std::forward<decltype(args)>(args)...);
+        run_one_policy(alloc, oneapi::dpl::execution::unseq, algo, checker, std::forward<decltype(args)>(args)...);


args are forwarded multiple times.

dmitriy-sobolev · 2025-06-16T21:23:45Z

test/parallel_api/ranges/std_ranges_memory.pass.cpp

+
+#if _ENABLE_STD_RANGES_TESTING
+
+#include "std_ranges_memory_test.h"


There is no much sense to to have a separate header file - it is small, and only on cpp file uses it.

MikeDvorskiy marked this pull request as draft June 2, 2025 15:41

MikeDvorskiy force-pushed the dev/mdvorski/ranges_algo_L3_memory-origin_UXL branch 4 times, most recently from 606f4fa to 39d183b Compare June 3, 2025 13:31

MikeDvorskiy marked this pull request as ready for review June 3, 2025 15:18

MikeDvorskiy force-pushed the dev/mdvorski/ranges_algo_L3_memory-origin_UXL branch 3 times, most recently from 22f1c16 to 0ec2d06 Compare June 4, 2025 15:11

MikeDvorskiy requested a review from dmitriy-sobolev June 4, 2025 16:05

MikeDvorskiy force-pushed the dev/mdvorski/ranges_algo_L3_memory-origin_UXL branch from 0ec2d06 to 470f339 Compare June 10, 2025 14:46

MikeDvorskiy requested review from SergeyKopienko, rarutyun, danhoeflinger and akukanov June 11, 2025 13:21

SergeyKopienko reviewed Jun 11, 2025

View reviewed changes

test/parallel_api/ranges/std_ranges_memory_test.h Outdated Show resolved Hide resolved

SergeyKopienko reviewed Jun 11, 2025

View reviewed changes

MikeDvorskiy force-pushed the dev/mdvorski/ranges_algo_L3_memory-origin_UXL branch 2 times, most recently from ab2357f to ee23b56 Compare June 11, 2025 15:59

MikeDvorskiy added 8 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

MikeDvorskiy added 2 commits June 12, 2025 12:28

[oneDPL][ranges][L3][test] + typo fix in the comments

75286a3

[oneDPL][ranges][L3] + fixes

9aad8b3

MikeDvorskiy force-pushed the dev/mdvorski/ranges_algo_L3_memory-origin_UXL branch from ee23b56 to 9aad8b3 Compare June 12, 2025 10:28

MikeDvorskiy added 22 commits June 12, 2025 12:31

Revert "[oneDPL][ranges][L1] host part, + static_assert(__is_parallel…

d3b67d8

…_tag_v<_Tag> || typename _Tag::__is_vector{}); and usage of some predefined functors" This reverts commit 0c2f14c.

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

fcfb0f2

This reverts commit 0740bda.

Revert "[oneDPL][ranges][L1] + ranges __pattern_reverse"

65c90c5

This reverts commit e7b7b63.

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

ed285f1

…l::__internal::__swap_fn" This reverts commit b725006.

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

1e74753

This reverts commit 81fd65a.

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

ba567e0

This reverts commit 4f8f8f5.

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

78e92b7

This reverts commit dab8e9a.

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

f9fd862

This reverts commit 72b1f8e.

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

2e981be

This reverts commit 6fdd2cb.

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

46a9f2b

This reverts commit 31a659c.

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

981f68c

This reverts commit 786044c.

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

b3ba3bc

This reverts commit dfee286.

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

86136d3

This reverts commit 4aaa892.

Revert "[oneDPL][ranges][L1] removed set algo range api; it is moving…

35c4698

… to another PR" This reverts commit 683585e.

Revert "[oneDPL][ranges][L1] + ranges::swap_ranges, __ranges::pattern…

6392df7

…_swap" This reverts commit 8e8a1c5.

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

3e3df3f

This reverts commit ce723c2.

Revert "[oneDPL][ranges][L1] + usage of oneapi::dpl::__internal::__co…

c009174

…mpare" This reverts commit a3dc444.

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

808cc9d

This reverts commit d4048e2.

Revert "[oneDPL][ranges][L1] + ranges::set_difference, ranges::set_se…

660b5a6

…t_symmetric_difference, high level API" This reverts commit 42a98e8.

Revert "[oneDPL][ranges][L1] + ranges::includes, ranges::set_union, r…

9e74f88

…anges::set_intersection high level API" This reverts commit c3f437e.

Revert "[oneDPL][ranges][L1] + ranges::unique, ranges::unique_copy, h…

24ea1c5

…igh level API" This reverts commit ac103a1.

Revert "[oneDPL][ranges][L1] + ranges::reverse, ranges::reverse_copy,…

3db16f1

… high level API" This reverts commit d30bb56.

SergeyKopienko reviewed Jun 12, 2025

View reviewed changes

dmitriy-sobolev reviewed Jun 16, 2025

View reviewed changes


		oneapi::dpl::uninitialized_fill(std::forward<_ExecutionPolicy>(__exec), __first, __last, __value);

		return {__last};


		oneapi::dpl::destroy(std::forward<_ExecutionPolicy>(__exec), __first, __last);

		return {__last};

	requires std::constructible_from<std::ranges::range_value_t<_InRange>, std::ranges::range_reference_t<_OutRange>>
	requires std::constructible_from<std::ranges::range_value_t<_OutRange>, std::ranges::range_reference_t<_InRange>>

		template<typename _S, typename _I>
		concept nothrow_sentinel_for = std::sentinel_for<_S, _I>;

		run_one_policy(alloc, oneapi::dpl::execution::seq, algo, checker, std::forward<decltype(args)>(args)...);
		run_one_policy(alloc, oneapi::dpl::execution::unseq, algo, checker, std::forward<decltype(args)>(args)...);


		#if _ENABLE_STD_RANGES_TESTING

		#include "std_ranges_memory_test.h"

[onedpl][ranges] L3 Range based algo API and implementation (memory group) #2292

Are you sure you want to change the base?

[onedpl][ranges] L3 Range based algo API and implementation (memory group) #2292

Uh oh!

Conversation

MikeDvorskiy commented Jun 2, 2025

Uh oh!

Uh oh!

SergeyKopienko Jun 11, 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!

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!

dmitriy-sobolev Jun 13, 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!

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!

dmitriy-sobolev Jun 12, 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!

SergeyKopienko Jun 12, 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitriy-sobolev Jun 16, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SergeyKopienko Jun 11, 2025 •

edited

Loading

dmitriy-sobolev Jun 13, 2025 •

edited

Loading

dmitriy-sobolev Jun 12, 2025 •

edited

Loading

SergeyKopienko Jun 12, 2025 •

edited

Loading

dmitriy-sobolev Jun 16, 2025 •

edited

Loading