[libcxx-commits] [PATCH] D151717: [libc++][PSTL] Add a GCD backend

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/include/__algorithm/pstl_backends/cpu_backends/libdispatch.h:112-116
+  if (__size1 + __size2 <= __default_chunk_size) {
+    __ranges.emplace_back(
+        std::move(__first1), std::move(__last1), std::move(__first2), std::move(__last2), std::move(__result));
+    return;
+  }
----------------
ldionne wrote:
> We should ensure that this `emplace_back()` doesn't need to reallocate, otherwise it gets messy with exceptions (and it's a bit inefficient). We should add an assertion here `_LIBCPP_ASSERT_UNCATEGORIZED(__ranges.capacity() >= __ranges.size() + 1);`.
> 
Not applicable anymore.


================
Comment at: libcxx/include/__utility/terminate_on_exception.h:30-31
     return __func();
+  } catch (const __pstl_bad_alloc&) {
+    throw;
   } catch (...) {
----------------
ldionne wrote:
> I'm not sure this is correct as-is. Let's say we have a predicate to `find_if` that happens to use `__pstl_merge` in its implementation, and let's say the "setup" code for `__pstl_merge` fails to allocate and throws `__pstl_bad_alloc`. It will be caught by `find_if`'s `__terminate_on_exception` wrapper and be rethrown. Right? If so, then we'd be incorrectly propagating the exception.
> 
> Instead, I think we might want to instead use `__terminate_on_exception` in the backend implementation *only* around user code (not setup code), so we'd remove it from the various functions that take `__cpu_backend_tag`. It would also not need to handle `__pstl_bad_alloc` anymore cause it would never wrap "setup" code.
> 
> This should be done in a separate patch, and we'll likely need some tests as well.
See D154238.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.merge/pstl.merge.pass.cpp:99
       std::vector<int> out(std::size(a) + std::size(b));
-      std::merge(
-          Iter1(a.data()), Iter1(a.data() + a.size()), Iter2(b.data()), Iter2(b.data() + b.size()), std::begin(out));
+      std::merge(policy,
+                 Iter1(a.data()),
----------------
ldionne wrote:
> Let's do this as a separate patch.
See D154546.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151717