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

Ruslan Arutyunyan via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Sun Sep 26 17:05:21 PDT 2021


rarutyun added inline comments.


================
Comment at: pstl/include/pstl/internal/omp/parallel_for.h:64
+        _PSTL_PRAGMA(omp parallel)
+        _PSTL_PRAGMA(omp single) { __pstl::__omp_backend::__parallel_for_body(__first, __last, __f); }
+    }
----------------
Please modify to `omp single nowait` here and in all other places where `omp single` is used within `omp parallel`. Currently we have two barriers from `parallel` and `single`. The inner barrier may be eliminated.


================
Comment at: pstl/include/pstl/internal/omp/parallel_merge.h:74
+{
+    std::size_t __size_x = std::distance(__xs, __xe);
+    std::size_t __size_y = std::distance(__ys, __ye);
----------------
MikeDvorskiy wrote:
> The "parallel merge" pattern accepts  just random access iterator types. So, we can write here just difference operator, everywhere in the patch.
> 
> Probably, it makes sense to add an assert (in the begin of the function) on random "access iterator type".
I would even say we **should** write `__xe - __xs` because `std::distance` works with any iterator type. Since we expect random access iterator only to be there we might hide the error with the code as it is. In case of using `operator-` explicitly we get the compile-time error immediately if this very operator is not defined. And one more thing. Using additional abstraction layers in places where we don't need them (in this case slightly, but) increases compilation time.


================
Comment at: pstl/include/pstl/internal/omp/parallel_stable_sort.h:25
+{
+template <typename Iterator1, typename Iterator2>
+struct __move_value
----------------
Why did you prefer to make the whole class template rather than making `operator()` with template parameters?


================
Comment at: pstl/include/pstl/internal/omp/parallel_stable_sort.h:29
+    void
+    operator()(Iterator1 __x, Iterator2 __z)
+    {
----------------
`const` at the end please


================
Comment at: pstl/include/pstl/internal/omp/parallel_stable_sort.h:37
+Iterator2
+__parallel_move_range(Iterator1 __first1, Iterator1 __last1, Iterator2 __first2)
+{
----------------
__first2 supposed to be `OutputIterator` semantically. Let's rename the second template parameter to `OutputIterator` and the last function parameter to `__d_first` or something. Otherwise, readability suffers. 


================
Comment at: pstl/include/pstl/internal/omp/parallel_stable_sort.h:66
+
+template <typename Iterator1, typename Iterator2>
+struct __move_range
----------------
This comment is applicable if the callable is required. Why do you need to be templatized? Could those template parameters be tied to `operator()`


================
Comment at: pstl/include/pstl/internal/omp/parallel_stable_sort.h:67
+template <typename Iterator1, typename Iterator2>
+struct __move_range
+{
----------------
Why do you need such a proxy object? Could you call `__parallel_move_range` directly?


================
Comment at: pstl/include/pstl/internal/omp/parallel_stable_sort.h:70
+    Iterator2
+    operator()(Iterator1 __first1, Iterator1 __last1, Iterator2 __first2)
+    {
----------------
If the Callable is required please mark method as const


================
Comment at: pstl/include/pstl/internal/omp/parallel_stable_sort.h:101
+        _VecType __output(__size);
+        _MoveValueType __move_value;
+        _MoveRangeType __move_range;
----------------
`__move_value` is named as the type (not the alias) that decreases readability. Wouldn't it be better to rename the variable (probably `__move_value_func`)? The same is true for ``__move_range``


================
Comment at: pstl/include/pstl/internal/omp/parallel_stable_sort.h:118
+void
+__parallel_stable_sort(_ExecutionPolicy&& __exec, _RandomAccessIterator __xs, _RandomAccessIterator __xe,
+                       _Compare __comp, _LeafSort __leaf_sort, std::size_t __nsort = 0)
----------------
I don't need `__exec` as a name, do you? I personally like the style with comments (`/*__exec */`) but it's up to you


================
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();
----------------
Don't see the reason why we need to add such an alias. In some places you use that in others - you don't


================
Comment at: pstl/include/pstl/internal/omp/util.h:60
+template <typename _Tp>
+class __buffer
+{
----------------
MikeDvorskiy wrote:
> I believe the call class __buffer can be shared between the all host backend.
> But, I think, for now is OK. It may be done later, in the one of next  patch.
I think it should not be a big deal to create the header with the `__buffer` class within and include it to both TBB backend and OpenMP backend. Otherwise we will commit relatively big code duplication 


================
Comment at: pstl/include/pstl/internal/omp/util.h:83
+// Preliminary size of each chunk: requires further discussion
+constexpr std::size_t __default_chunk_size = 512;
+
----------------
I am not sure if we need this variable basing on Mikhail's comments (probably, it's still useful for some algorithms) but if it remains in the code please mark it `inline`.


================
Comment at: pstl/include/pstl/internal/omp/util.h:88
+constexpr auto
+__should_run_serial(_Iterator __first, _Iterator __last) -> bool
+{
----------------
Is there any reason to use trailing return types? As I remember correctly you use those only in this file.


================
Comment at: pstl/include/pstl/internal/omp/util.h:99
+{
+    using _difference_type = ::std::intptr_t;
+    auto __size = __last - __first;
----------------
Why do you use `std::intptr_t`? it's a type to represent the pointer as the signed value. If you already check (with `enable_if`) that your type is integral and you have two same types (by function signature) you may case directly to `Index`, may not you?


================
Comment at: pstl/include/pstl/internal/omp/util.h:104
+
+struct __chunk_policy
+{
----------------
`chuck_policy` (a least to me) is a very confusing name in the context of Parallel STL. I had an impression that use have some special execution policy or you wrap `ExecutionPolicy` object with this function or something like that. Cannot propose better from the top of my head. Need to think a little bit about that.


================
Comment at: pstl/include/pstl/internal/omp/util.h:106
+{
+    std::size_t __n_chunks{0}, __chunk_size{0}, __first_chunk_size{0};
+};
----------------
I would recommend to split those variable to make each in its line for the sake of readability.


================
Comment at: pstl/include/pstl/internal/omp/util.h:106
+{
+    std::size_t __n_chunks{0}, __chunk_size{0}, __first_chunk_size{0};
+};
----------------
rarutyun wrote:
> I would recommend to split those variable to make each in its line for the sake of readability.
If I don't miss something you always construct `chunk_policy` object with 3 arguments. Why do you need field initialization? I would prefer to leave this struct `trivially_default_constructible` unless we have strong reasons not to do that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99836



More information about the libcxx-commits mailing list