oneDPL
oneDPL copied to clipboard
Add workaround to parallel merge in TBB backend not to use input iterators default constructor
Add workaround to implementation of __parallel_merge function for the following use case:
#include <oneapi/dpl/algorithm>
#include <oneapi/dpl/execution>
#include <oneapi/dpl/iterator>
#include <vector>
int main() {
std::vector<int> input1 = {1, 2, 3};
std::vector<int> input2 = {1, 2, 3};
std::vector<int> output = {0, 0, 0};
auto no_transformation = [](int i) { return i; };
oneapi::dpl::merge(oneapi::dpl::execution::par,
oneapi::dpl::make_transform_iterator(input1.begin(), no_transformation),
oneapi::dpl::make_transform_iterator(input1.end(), no_transformation),
input2.begin(),
input2.end(),
output.begin());
}
The first input iterator in merge
algorithm is dpl::transform_iterator<vector_it, lambda>
. Since lambda is not default constructible, this use case results in the compilation error while trying to default construct iterator inside TBB backend.
From the C++ Standard perspective, it is not an issue, because default constructability is part of ForwardIterator requirements, but after the short offline discussion, it was decided to workaround this place in parallel backend TBB to make things better for our fancy iterators.
There is still no 100% guarantee, that no issues would be present for transform iterator with lambda body, because some algorithms we use from the C++ Standard Library are still allowed to call default constructor on the input iterator, e.g. std::upper_bound
). This PR was tested together with libstdc++ and no issues was found.
I think we are able to fix this issue without changes in transform_iterator
and it's more preferable: for that we should duplicate some code in if
/ else
parts inside https://github.com/oneapi-src/oneDPL/pull/1346/files#diff-9908a4470cd112b8a7c7c87c5d3dfa425d873a5e5f9bf353268e6bdf756f5af8.
What about the code
// Using callable object instead of lambda here to ensure transform iterator would be
// default constructible, that is part of the Forward Iterator requirements in the C++ standard.
struct NoTransform
{
ValueType operator()(const ValueType& val) const
{
return val;
}
};
Should we use lambda now in https://github.com/oneapi-src/oneDPL/blob/5740f585386bed798af83e1c18ae1ee92c613637/test/parallel_api/iterator/permutation_iterator.h#L211 ?
What about the code
// Using callable object instead of lambda here to ensure transform iterator would be // default constructible, that is part of the Forward Iterator requirements in the C++ standard. struct NoTransform { ValueType operator()(const ValueType& val) const { return val; } };
Should we use lambda now in
https://github.com/oneapi-src/oneDPL/blob/5740f585386bed798af83e1c18ae1ee92c613637/test/parallel_api/iterator/permutation_iterator.h#L211
?
I would prefer to keep this test as-is because, as I mentioned this fix is not a 100% guarantee that merge with transform_iterator with lambda body would work. std::upper_bound
and std::lower_bound
implementations can still rely on default constructability of input iterators.
For today, this code works, but formally such an iterator violates the ForwardIterator requirements from the Standard.
This patch is intended to minimize the risk of issues, as we discussed offline.
What about the code
// Using callable object instead of lambda here to ensure transform iterator would be // default constructible, that is part of the Forward Iterator requirements in the C++ standard. struct NoTransform { ValueType operator()(const ValueType& val) const { return val; } };
Should we use lambda now in https://github.com/oneapi-src/oneDPL/blob/5740f585386bed798af83e1c18ae1ee92c613637/test/parallel_api/iterator/permutation_iterator.h#L211
?
I would prefer to keep this test as-is because, as I mentioned this fix is not a 100% guarantee that merge with transform_iterator with lambda body would work.
std::upper_bound
andstd::lower_bound
implementations can still rely on default constructability of input iterators. For today, this code works, but formally such an iterator violates the ForwardIterator requirements from the Standard. This patch is intended to minimize the risk of issues, as we discussed offline.
ok
Should the patch also be done in PSTL upstream? @MikeDvorskiy @rarutyun