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

[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/CMakeLists.txt:792
   config_define(1 _LIBCPP_PSTL_CPU_BACKEND_THREAD)
+elseif(LIBCXX_PSTL_CPU_BACKEND STREQUAL "libdispatch")
+  config_define(1 _LIBCPP_PSTL_CPU_BACKEND_LIBDISPATCH)
----------------
We need `set(LIBCXX_PSTL_CPU_BACKEND libdispatch)` inside `libcxx/cmake/caches/Apple.cmake`


================
Comment at: libcxx/include/__algorithm/pstl_backends/cpu_backends/libdispatch.h:12
+
+#include <__algorithm/lower_bound.h>
+#include <__algorithm/upper_bound.h>
----------------
Missing include for `move_iterator`.


================
Comment at: libcxx/include/__algorithm/pstl_backends/cpu_backends/libdispatch.h:130-134
+      __merge_range_t __ret{__mid1, __mid2, __result};
+      __result += (__mid1 - __first1) + (__mid2 - __first2);
+      __first1 = __mid1;
+      __first2 = __mid2;
+      return __ret;
----------------
If you use `__first_iters` to store the result, this becomes:

```
__result += (__mid1 - __first1) + (__mid2 - __first2);
__first1 = __mid1;
__first2 = __mid2;
return __merge_range_t{__first1, __first2, __result};
```

And above becomes 
```
__ranges.emplace_back(__first1, __first2, __result);
```

instead of

```
__ranges.emplace_back(__first1, __first2, _RandomAccessIterator3{});
```


================
Comment at: libcxx/include/__algorithm/pstl_backends/cpu_backends/libdispatch.h:141-142
+    // handle 2 -> N - 1 chunks
+    for (ptrdiff_t __i = 1; __i != __partitions.__chunk_count_ - 1; ++__i)
+      __ranges.emplace_back(__compute_chunk(__partitions.__chunk_size_));
+
----------------
It makes it clearer that we do `N-1` iterations, and also that we require `__partitions.__chunk_count_ > 1` (which is the case, see above).


================
Comment at: libcxx/include/__algorithm/pstl_backends/cpu_backends/libdispatch.h:147
+
+    __libdispatch::__dispatch_apply(__ranges.size() - 1, [&](size_t __index) {
+      auto __first_iters = __ranges[__index];
----------------
IMO that's easier to read, and it's equivalent.


================
Comment at: libcxx/src/CMakeLists.txt:318-319
+if (LIBCXX_PSTL_CPU_BACKEND STREQUAL "libdispatch")
+  set(LIBCXX_EXPERIMENTAL_SOURCES ${LIBCXX_EXPERIMENTAL_SOURCES}
+                                  pstl/libdispatch.cpp)
+endif()
----------------
ldionne wrote:
> 
I don't think this was done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151717