[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_backends/cpu_backends/any_of.h:63
   }
   return false;
 }
----------------
We should document how backends are expected to handle exceptions in `libcxx/include/__algorithm/pstl_backend.h` and in `libcxx/include/__algorithm/pstl_backends/cpu_backend.h`.


================
Comment at: libcxx/include/__algorithm/pstl_backends/cpu_backends/find_if.h:92
 
 template <class _ExecutionPolicy, class _ForwardIterator, class _Predicate>
 _LIBCPP_HIDE_FROM_ABI _ForwardIterator
----------------
Notes from our 1:1 session:

```
// Option 1:
// Pros:
// - CPU sub-backends don't have to handle user exceptions in any special way. They only have to care
//   about any exception that might be thrown in their own setup code (aka std::bad_alloc).
// - Handling of excetpions at the `__pstl_foo` level is uniform across all algorithms.
// - We can write all code inside the PSTL with the mindset that exceptions are disabled,
//   i.e. we never throw and we use appropriate types to report errors instead. This might be
//   a nice systematic approach?
// Cons:
// - Requires introducing a custom std::expected-like type for internal use.
// - Hardcodes the fact that we can only throw `std::bad_alloc` from backends.
// - We can't elide the move construction of the return value out of `__pstl_find_if`.
template <class _ExecutionPolicy, class _ForwardIterator, class _Predicate>
_LIBCPP_HIDE_FROM_ABI _ForwardIterator
__pstl_find_if(__cpu_backend_tag, _ForwardIterator __first, _ForwardIterator __last, _Predicate __pred) {
  auto __maybe_result = std::__terminate_on_exception([&] -> __funky_expected<_ForwardIterator, std::bad_alloc> {
    if constexpr (__is_parallel_execution_policy_v<_ExecutionPolicy> &&
                  __has_random_access_iterator_category<_ForwardIterator>::value) {
      return std::__parallel_find(
          __first,
          __last,
          [&__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);
          },
          less<>{},
          true);
    } else if constexpr (__is_unsequenced_execution_policy_v<_ExecutionPolicy> &&
                        __has_random_access_iterator_category<_ForwardIterator>::value) {
      using __diff_t = __iter_diff_t<_ForwardIterator>;
      return std::__simd_first(__first, __diff_t(0), __last - __first, [&__pred](_ForwardIterator __iter, __diff_t __i) {
        return __pred(__iter[__i]);
      });
    } else {
      return std::find_if(__first, __last, __pred);
    }
  });
  return *std::move(__maybe_result).or_else([](auto&& __exception) -> __funky_expected<_ForwardIterator, void> { std::__throw_bad_alloc(); });
}

// Option 2:
// Pros:
// - Each layer fully handles all of its exceptions independently. That can be simpler or more complicated depending
//   on how you look at it.
// Cons:
// - When you're writing a CPU backend, you need to explicitly handle user-thrown exceptions and carve out the
//   setup-thrown exceptions from the `terminate_on_exception` code paths.
template <class _ExecutionPolicy, class _ForwardIterator, class _Predicate>
_LIBCPP_HIDE_FROM_ABI _ForwardIterator
__pstl_find_if(__cpu_backend_tag, _ForwardIterator __first, _ForwardIterator __last, _Predicate __pred) {
  if constexpr (__is_parallel_execution_policy_v<_ExecutionPolicy> &&
                __has_random_access_iterator_category<_ForwardIterator>::value) {
    return std::__parallel_find( // we would document that CPU sub-backends need to handle exceptions appropriately
        __first,
        __last,
        [&__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);
        },
        less<>{},
        true);
  } else if constexpr (__is_unsequenced_execution_policy_v<_ExecutionPolicy> &&
                      __has_random_access_iterator_category<_ForwardIterator>::value) {
    using __diff_t = __iter_diff_t<_ForwardIterator>;
    return std::__terminate_on_exception([&] {
      return std::__simd_first(__first, __diff_t(0), __last - __first, [&__pred](_ForwardIterator __iter, __diff_t __i) {
        return __pred(__iter[__i]);
      });
    });
  } else {
    return std::__terminate_on_exception([&] {
      return std::find_if(__first, __last, __pred);
    });
  }
}
```

At the end, we both agreed that option 2 is the way to go for now, largely because implementing an `expected`-like type in C++17 drives the cost of option 1 up a lot.



================
Comment at: libcxx/include/__algorithm/pstl_backends/cpu_backends/find_if.h:109-111
     return std::__simd_first(__first, __diff_t(0), __last - __first, [&__pred](_ForwardIterator __iter, __diff_t __i) {
       return __pred(__iter[__i]);
     });
----------------
If an exception is thrown in here, we would also need to terminate. This needs to be fixed + a test.


================
Comment at: libcxx/include/__algorithm/pstl_backends/cpu_backends/find_if.h:113
   } else {
     return std::find_if(__first, __last, __pred);
   }
----------------
If an exception is thrown in here, we should also terminate even though this is actually running serial. This needs fix + tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154238