[libcxx-commits] [PATCH] D59791: [pstl] Add a serial backend for the PSTL

[Mikhail Dvorskiy via Phabricator via libcxx-commits]

MikeDvorskiy added inline comments.


================
Comment at: pstl/include/pstl/internal/parallel_backend_serial.h:106
+{
+    __leaf_merge(__first1, __last1, __first2, __last2, __out, __comp);
+}
----------------
ldionne wrote:
> MikeDvorskiy wrote:
> > ldionne wrote:
> > > Note that the obvious implementation doesn't work:
> > > 
> > > ```
> > > std::merge(__first1, __last1, __first2, __last2, __out, __comp);
> > > ```
> > > 
> > > because it requires the elements to be copyable, but pstl apparently expects the merge to only move elements around without copying them.
> > Why "doesnt work"?
> > 
> > In special case
> > ...
> >     if (__n <= __merge_cut_off)
> >     {
> >         // Fall back on serial merge
> >         __leaf_merge(__xs, __xe, __ys, __ye, __zs, __comp);
> >     }
> > ...
> > where __leaf_merge is std::merge
> Like I said, `std::merge` requires elements to be copyable, but the PSTL tests call `parallel_merge` with elements that are move-only. It also looks like you guys went through some hoops to make this work, see `__serial_move_merge` in `parallel_backend_utils.h`.
> 
> However, since it looks like calling `__leaf_merge` is a valid implementation, let's revisit this copyability issue at another point. I want to land this patch ASAP because other stuff depends on it.
> 
> 
> In case you're curious, the error message for copyability looks like this:
> ```
> <toolchain>/usr/include/c++/v1/algorithm:4392:23: error: object of type 'LocalWrapper<float>' cannot be assigned because its copy assignment operator is implicitly deleted
>             *__result = *__first2;
>                       ^
> <toolchain>/usr/include/c++/v1/algorithm:4416:19: note: in instantiation of function template specialization 'std::__1::__merge<std::__1::less<LocalWrapper<float> > &, std::__1::__wrap_iter<LocalWrapper<float> *>, std::__1::__wrap_iter<LocalWrapper<float> *>, LocalWrapper<float> *>' requested here
>     return _VSTD::__merge<_Comp_ref>(__first1, __last1, __first2, __last2, __result, __comp);
>                   ^
> <pstl-root>/include/pstl/internal/parallel_backend_serial.h:117:10: note: in instantiation of function template specialization 'std::__1::merge<std::__1::__wrap_iter<LocalWrapper<float> *>, std::__1::__wrap_iter<LocalWrapper<float> *>, LocalWrapper<float> *, std::__1::less<LocalWrapper<float> > >' requested here
>     std::merge(__first1, __last1, __first2, __last2, __out, __comp);
>          ^
> <pstl-root>/include/pstl/internal/algorithm_impl.h:2667:24: note: in instantiation of function template specialization '__pstl::__serial::__parallel_merge<const __pstl::execution::v1::parallel_policy &, std::__1::__wrap_iter<LocalWrapper<float> *>, std::__1::__wrap_iter<LocalWrapper<float> *>, LocalWrapper<float> *, std::__1::less<LocalWrapper<float> >, (lambda at <pstl-root>/include/pstl/internal/algorithm_impl.h:2669:13)>' requested here
>         __par_backend::__parallel_merge(
>                        ^
> <pstl-root>/include/pstl/internal/glue_algorithm_impl.h:899:17: note: in instantiation of function template specialization '__pstl::__internal::__pattern_inplace_merge<const __pstl::execution::v1::parallel_policy &, std::__1::__wrap_iter<LocalWrapper<float> *>, std::__1::less<LocalWrapper<float> >, std::__1::integral_constant<bool, false> >' requested here
>     __internal::__pattern_inplace_merge(
>                 ^
> <pstl-root>/test/std/algorithms/alg.merge/inplace_merge.pass.cpp:63:14: note: in instantiation of function template specialization 'std::inplace_merge<const __pstl::execution::v1::parallel_policy &, std::__1::__wrap_iter<LocalWrapper<float> *>, std::__1::less<LocalWrapper<float> > >' requested here
>         std::inplace_merge(exec, first2, mid2, last2, comp);
>              ^
> <pstl-root>/test/support/utils.h:757:9: note: (skipping 2 contexts in backtrace; use -ftemplate-backtrace-limit=0 to see all)
>         op(std::forward<Rest>(rest)...);
>         ^
> ...
> ```
> 
1. Yes, std::merge requires "copyable".

Actually, the all "move" operations  in "__serial_move_merge"  will reduce to "copy" operations If "move" semantic is not specified.

But, you are right. If we pass non-const iterator into std::merge and a value type has the non-trivial move semantic we have got wrong effect... 
It may be quickly  fixed in "__parallel_merge". I'll do it. 

2. But the compiler diagnostic shown here tells about "std::inplace_merge" algo. This algo requires just move semantic and the test checks it.
The problem is that __pattern_inplace_merge re-uses  __par_backend::__parallel_merge due to there is no a special __par_backend::__parallel__inplace_merge API. I think it makes sense to add __par_backend::__parallel__inplace_merge API (and move the relevant code from __pattern_inplace_merge into __par_backend). I'll do it. After that in your serial back-end you should just call std::inplace_merge serial. 

Thanks for raising the issues.




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59791/new/

https://reviews.llvm.org/D59791