[libcxx-commits] [PATCH] D99836: [pstl] Implement OpenMP backend

[Mikhail Dvorskiy via Phabricator via libcxx-commits]

MikeDvorskiy requested changes to this revision.
MikeDvorskiy added a comment.
This revision now requires changes to proceed.

Christopher, one more comment - we discussed and agreed that a better name for macro _PSTL_PAR_BACKEND_OMP  is

> _PSTL_PAR_BACKEND_OPENMP

(I've counted 3 matches in the patch.)



================
Comment at: pstl/include/pstl/internal/omp/parallel_merge.h:53
+                     firstprivate(__xs, __xm, __ys, __ym, __zs, __comp, __leaf_merge))
+    __parallel_merge_body((__xm - __xs), (__ym - __ys), __xs, __xm, __ys, __ym, __zs, __comp, __leaf_merge);
+
----------------
Talking about the redundant  parentheses on line 45 - yes, perhaps, I can agree that it improves readability... But, here, on the line 53, I guess, it worsens readability, IMHO.

BTW,
On the line 103 the similar code written without redundant  parentheses - 
```
__parallel_merge_body(
            __mid - __xs, __xe - __mid,...
```


================
Comment at: pstl/include/pstl/internal/omp/parallel_stable_sort.h:37
+{
+    std::size_t __size = std::distance(__first1, __last1);
+
----------------
According to my and Ruslan's comments about usage of "std::distance"  in case of random access iterators - yes, as we agreed, you have changed it by difference.... but why here (and on line 55) it was kept? 


================
Comment at: pstl/include/pstl/internal/omp/parallel_transform_reduce.h:83
+template <class _ExecutionPolicy, class _RandomAccessIterator, class _UnaryOp, class _Value, class _Combiner,
+          class _Reduct if (__first == __last)
+{
----------------
The lines (83-87) look  a kind of "merge artifacts"....


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99836