[libcxx-commits] [PATCH] D154238: [libc++][PSTL] Move exception handling to the CPU backends

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__algorithm/pstl_backend.h:78
   template <class _ExecutionPolicy, class _Iterator, class _Size, class _Func>
   void __pstl_for_each_n(_Backend, _Iterator __first, _Size __n, _Func __f);
 
----------------
These should be updated for `optional<foo>`


================
Comment at: libcxx/include/__algorithm/pstl_backend.h:163
+parallel algorithm which is invoked inside another algorithm. If any allocation fails, the backend shall return a
+__pstl_unexpected, which will be turned into a bad_alloc at the frontend level.
 */
----------------
`a disengaged optional`. Same below.


================
Comment at: libcxx/include/__algorithm/pstl_backends/cpu_backend.h:51
+
+The backend is expected to terminate on any user-thrown exceptions. This includes exceptions which are thrown by a
+parallel algorithm which is invoked inside another algorithm. If any allocation fails, the backend shall throw
----------------
ldionne wrote:
> 
Actually, I think we want to say:

```
CPU backends are expected to report errors (i.e. failure to allocate) by returning a disengaged `optional` from their implementation. Exceptions shouldn't be used to report an internal failure-to-allocate, since all exceptions are turned into a program termination at the front-end level. When a backend returns a disengaged `optional` to the frontend, the frontend will turn that into a call to `std::__throw_bad_alloc();` to report the internal failure to the user.
```



================
Comment at: libcxx/include/__algorithm/pstl_backends/cpu_backends/find_if.h:105
+        [&__pred](_ForwardIterator __brick_first, _ForwardIterator __brick_last) {
+          return *std::__pstl_find_if<__remove_parallel_policy_t<_ExecutionPolicy>>(
+              __cpu_backend_tag{}, __brick_first, __brick_last, __pred);
----------------
Let's add an internal assertion here to document why `__pstl_find_if` will never return nullopt (the reason is that `unseq` isn't allowed to allocate, assuming that's true).

If that's not true, then this is a bug.


================
Comment at: libcxx/include/__algorithm/pstl_backends/cpu_backends/libdispatch.h:66
+_LIBCPP_HIDE_FROM_ABI optional<__empty>
+__parallel_for(_RandomAccessIterator __first, _RandomAccessIterator __last, _Functor __func) noexcept {
   auto __partitions = __libdispatch::__partition_chunks(__last - __first);
----------------
Per our discussion just now, we should actually remove `noexcept` here!


================
Comment at: libcxx/include/__algorithm/pstl_backends/cpu_backends/transform_reduce.h:166
+        [__transform, __reduce](auto __brick_first, auto __brick_last, _Tp __brick_init) {
+          return *std::__pstl_transform_reduce<__remove_parallel_policy_t<_ExecutionPolicy>>(
+              __cpu_backend_tag{},
----------------
Same, internal assertion.


================
Comment at: libcxx/include/__algorithm/pstl_for_each.h:58
 _LIBCPP_HIDE_FROM_ABI void
 for_each_n(_ExecutionPolicy&& __policy, _ForwardIterator __first, _Size __size, _Function __func) {
   _LIBCPP_REQUIRE_CPP17_FORWARD_ITERATOR(_ForwardIterator);
----------------
As discussed, it looks like we actually need to do this (otherwise we'd terminate in case `std::for_each(policy, ...)` throws `bad_alloc`):

```
template <class _ExecutionPolicy,
          class _ForwardIterator,
          class _Size,
          class _Function,
          class _RawPolicy                                    = __remove_cvref_t<_ExecutionPolicy>,
          enable_if_t<is_execution_policy_v<_RawPolicy>, int> = 0>
_LIBCPP_HIDE_FROM_ABI void
__for_each_n(_ExecutionPolicy&& __policy, _ForwardIterator __first, _Size __size, _Function __func) noexcept {
  _LIBCPP_REQUIRE_CPP17_FORWARD_ITERATOR(_ForwardIterator);
  return std::__pstl_frontend_dispatch(
      _LIBCPP_PSTL_CUSTOMIZATION_POINT(__pstl_for_each_n),
      [&](_ForwardIterator __g_first, _Size __g_size, _Function __g_func) -> optional<__empty> {
        if constexpr (__has_random_access_iterator_category_or_concept<_ForwardIterator>::value) {
          std::for_each(__policy, std::move(__g_first), __g_first + __g_size, std::move(__g_func));
          return __empty{};
        } else {
          std::for_each_n(std::move(__g_first), __g_size, std::move(__g_func));
          return __empty{};
        }
      },
      __first,
      __size,
      std::move(__func));
}

template <class _ExecutionPolicy,
          class _ForwardIterator,
          class _Size,
          class _Function,
          class _RawPolicy                                    = __remove_cvref_t<_ExecutionPolicy>,
          enable_if_t<is_execution_policy_v<_RawPolicy>, int> = 0>
_LIBCPP_HIDE_FROM_ABI void
for_each_n(_ExecutionPolicy&& __policy, _ForwardIterator __first, _Size __size, _Function __func) {
  auto __res = std::__for_each_n(...);
  if (!__res)
    std::__throw_bad_alloc();
}
```

And then we should use the `noexcept` version from fallback implementations.


================
Comment at: libcxx/include/__utility/terminate_on_exception.h:25
 template <class _Func>
-_LIBCPP_HIDE_FROM_ABI auto __terminate_on_exception(_Func __func) {
-  try {
+_LIBCPP_HIDE_FROM_ABI auto __terminate_on_exception(_Func __func) noexcept {
     return __func();
----------------
I think this is just not needed at all anymore.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154238