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

[Christopher Nelson via Phabricator via libcxx-commits]

nadiasvertex marked 10 inline comments as done.
nadiasvertex added a comment.

Addressed all known concerns with the last 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);
+
----------------
MikeDvorskiy wrote:
> 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,...
> ```
I did a search and replace. Those parentheses were a side-effect of a mistake I made in the regular expression. Sorry. This is fixed now.


================
Comment at: pstl/include/pstl/internal/omp/parallel_stable_sort.h:37
+{
+    std::size_t __size = std::distance(__first1, __last1);
+
----------------
MikeDvorskiy wrote:
> 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? 
Yup. It wasn't clear to me that this was also required to be a random access iterator. I have made the change here too.


================
Comment at: pstl/include/pstl/internal/omp/parallel_transform_reduce.h:37
+{
+    using _Size = std::size_t;
+    const _Size __num_threads = omp_get_num_threads();
----------------
nadiasvertex wrote:
> rarutyun wrote:
> > Don't see the reason why we need to add such an alias. In some places you use that in others - you don't
> Isn't calling omp_get_num_threads() slower than reading a variable?
I misunderstood what you were talking about. I have fixed this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99836