[libcxx-commits] [PATCH] D149706: [libc++][PSTL] Implement std::copy{, _n}

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

LGTM w/ comments and CI.



================
Comment at: libcxx/include/__algorithm/pstl_backend.h:25-26
 /*
 TODO: Documentation of how backends work
 
 A PSTL parallel backend is a tag type to which the following functions are associated, at minimum:
----------------
Let's add something like:

```
PSTL algorithms are a thin API layer that forward to a backend that implements the actual operation. The backend is selected based on the execution policy provided by the user.

All algorithms go through the dispatching mechanism, so a backend can customize any algorithm in the PSTL in a way that suits it. However, the only exception to this is when an algorithm can be implemented by a C function, we assume that the C function is the most efficient way to implement it on the platform and we use that unconditionally. For example, `std::copy(policy, ...)` will forward to `memmove` if the iterators and the copied type allow for it, regardless of whether the backend associated to `policy` implements `__pstl_copy`.

The following functions must be provided by all backends:
```


================
Comment at: libcxx/include/__algorithm/pstl_copy.h:41-48
+  return std::__pstl_frontend_dispatch(
+      _LIBCPP_PSTL_CUSTOMIZATION_POINT(__pstl_copy),
+      [&](_ForwardIterator __first, _ForwardIterator __last, _ForwardOutIterator __result) {
+        return std::transform(__policy, __first, __last, __result, __identity());
+      },
+      std::move(__first),
+      std::move(__last),
----------------
Here I would do something like

```
if constexpr (can-be-optimized-to-memmove(_ForwardIterator, ...)) {
  // ditch the execution policy and call the single-threaded std::copy, since we know it handles the complexity of optimizing properly
  return std::copy(__first, __last, __result);
} else {
  return std::__pstl_frontend_dispatch(...);
}
```

In a separate patch.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/pstl.copy.pass.cpp:11
+
+// REQUIRES: with-pstl
+
----------------
This isn't used anymore, and it means the test never ran!


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/pstl.copy.pass.cpp:98
+int main(int, char**) {
+  types::for_each(types::forward_iterator_list<int*>{}, TestIteratorsInt{});
+  types::for_each(types::forward_iterator_list<CopiedToTester*>{}, TestIteratorsNonTrivial{});
----------------
jloser wrote:
> philnik wrote:
> > ldionne wrote:
> > > I see this is repeating a lot, and because of C++17 we can't use template lambas. But we can do this:
> > > 
> > > ```
> > > types::for_each(types::forward_iterator_list<int*>{}, types::apply_type_identity([](auto It) {
> > >   using Iter = typename decltype(It)::type;
> > >   // the rest of your stuff
> > > }));
> > > 
> > > // where
> > > template <class F>
> > > struct apply_type_identity {
> > >   F f;
> > >   template <class ...Args>
> > >   auto operator()() const {
> > >     return f(std::type_identity<Args>{}...);
> > >   }
> > > };
> > > ```
> > > 
> > > We can bikeshed about the name of `apply_type_identity`, I'm open to anything reasonable. IMO if we apply this throughout, it will localize the "loops" and make the testing code easier to read (and write!). WDYT?
> > I'm not a huge fan of the name, but I can't come up with anything better.
> I think the name is fine FWIW. Do we have a generic type for applying a generic metafunction (e.g. so a call site could look like `apply<F, std::type_identity>`)? The metafunction would be bound with the `Args` parameter pack on the call operator like it is now.  Then, if you had such generic construct, you wouldn't need to name the entity `apply_type_identity`, it would be obvious from the callers.
The downside of `apply<F, std::type_identity>` is that `F` here is `decltype(lamda)`, and that doesn't work with Clang :/

So we'd have to introduce the lambda and then do `decltype(x)`, which is not as compositional.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/pstl.copy_n.pass.cpp:11
+
+// REQUIRES: with-pstl
+
----------------
x2


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/pstl.copy_n.pass.cpp:92
+  void operator()() {
+    types::for_each(types::forward_iterator_list<CopiedToTester*>{},
+                    TestIteratorWithPolicies<types::partial_instantiation<TestNonTrivial, Iter2>::template apply>{});
----------------
Same comment about lambdas.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149706