[libcxx-commits] [PATCH] D150277: [libc++][PSTL] Move the already implemented functions to the new dispatching scheme

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

Nice! LGTM once CI is green.



================
Comment at: libcxx/include/__algorithm/pstl_backends/cpu_backend.h:20
 
+  // Cancel the execution of other jobs - they aren't needed anymore
+  void __cancel_execution();
----------------



================
Comment at: libcxx/include/__algorithm/pstl_backends/cpu_backends/fill.h:38
+_LIBCPP_HIDE_FROM_ABI void
+__pstl_fill(_ExecutionPolicy&& __policy, _ForwardIterator __first, _ForwardIterator __last, const _Tp& __value) {
+  if constexpr (__is_parallel_execution_policy_v<_ExecutionPolicy> &&
----------------
ldionne wrote:
> Missing `__cpu_backend_tag`
We should have a test for the CPU backend that lists all the functions it expects to provide, and make sure that they can indeed be called. Otherwise, refactorings can produce stuff like:

```
template <class _ExecutionPolicy, class _ForwardIterator, class _Tp, class = enable_if_t<...> >

template <class _ExecutionPolicy, class _ForwardIterator, class _Tp, enable_if_t<..., int> = 0>

template <class _ExecutionPolicy, class _ForwardIterator, class _Tp, enable_if_t<...> >
```

which are never valid overloads. This is actually quite tricky and we've been bitten by this in the past. This can be done in a separate patch, but I think it's a requirement of this approach or else we risk introducing performance regressions.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150277