-
Notifications
You must be signed in to change notification settings - Fork 116
[onedpl][ranges] L1 Range based algo API and implementation #2272
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
base: main
Are you sure you want to change the base?
Conversation
6ef1d07
to
3b9fa92
Compare
b02531f
to
25b1761
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
return oneapi::dpl::ranges::copy(std::forward<_ExecutionPolicy>(__exec), std::ranges::reverse_view(__in_r), | ||
std::forward<_OutRange>(__out_r)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
__pattern_reverse_copy
from the device backend also uses an optimized __reverse_copy
routine. Let's reuse it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, it makes sense for hetero policy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update: The question was proceed with C++ committee and accepted into the C++ standard.
So, ranges::reverse_copy
should return 3 iterators: the last
in the input range, a last position
in the input range, the last
in the output range.
|
||
namespace __internal | ||
{ | ||
struct __reverse_copy_fn |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no a corresponding test. Could you add it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update: The question was proceed with C++ committee and accepted into the C++ standard.
So, ranges::reverse_copy should return 3 iterators: the last in the input range, a last position in the input range, the last in the output range.
Yes, a test should be added.
a53327b
to
c0d749c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Minor comment: not all |
@MikeDvorskiy could you you please check and fix in this PR the issue #2283 ? |
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
data_in_out_lim
checks only "out" range. I expect that a test for swap_ranges
should check both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
22e0a8c
to
5987bc0
Compare
6cc066b
to
0c2f14c
Compare
…et_intersection high level API
…ric_difference, high level API
…Tag> || typename _Tag::__is_vector{}); and usage of some predefined functors
0c2f14c
to
c0d8469
Compare
Now we use a functor: |
[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)