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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne marked 3 inline comments as done.
ldionne added a comment.

In D59791#1458089 <https://reviews.llvm.org/D59791#1458089>, @MikeDvorskiy wrote:

> > I think we all agreed that this was worth implementing
>
> Yes, we agreed. I'm ready to explain any details If you need.


See questions in this review.



================
Comment at: pstl/include/pstl/internal/parallel_backend_serial.h:86
+{
+    return __scan(_Index(0), __n, __init);
+}
----------------
So, I strongly suspect this implementation is incorrect. Can you draft what a correct serial implementation would be?


================
Comment at: pstl/include/pstl/internal/parallel_backend_serial.h:94
+    // TODO: What to do with __nsort?
+    __leaf_sort(__first, __last, __comp);
+}
----------------
Is `__nsort` the number of elements that should be sorted in the resulting range? IOW, this is a partial sort where you want to get the `n` smallest elements of the whole range in sorted order.

It's not clear to me how to achieve this without a `__leaf_sort` that itself accepts a `n` to only partially sort the first `n` elements. I mean, I could probably find a way to do it iteratively by sorting a bit more every time, but what I really want is to just call `std::partial_sort`. And actually, while we're at it, calling `std::partial_sort` isn't enough, since this sort needs to be stable. But we don't have a `std::stable_partial_sort`.


================
Comment at: pstl/include/pstl/internal/parallel_backend_serial.h:106
+{
+    __leaf_merge(__first1, __last1, __first2, __last2, __out, __comp);
+}
----------------
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.


Repository:
  rPSTL pstl

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

https://reviews.llvm.org/D59791