[libcxx-commits] [PATCH] D104492: [libc++][pstl] Implement tag dispatching

[Ruslan Arutyunyan via Phabricator via libcxx-commits]

rarutyun marked 3 inline comments as done.
rarutyun added inline comments.


================
Comment at: pstl/include/pstl/internal/algorithm_fwd.h:269
 _ForwardIterator
-__pattern_find_if(_ExecutionPolicy&&, _ForwardIterator, _ForwardIterator, _Predicate, _IsVector,
-                  /*is_parallel=*/std::false_type) noexcept;
+__pattern_find_if(_Tag __tag, _ExecutionPolicy&&, _ForwardIterator __first, _ForwardIterator __last,
+                  _Predicate __pred) noexcept;
----------------
Quuxplusone wrote:
> Throughout: On function declarations, I think it's unhelpful to have parameter names like `_Tag __tag` or (below) `_ExecutionPolicy&& __exec`. I would remove those parameter names. The names `__first, __last, __pred` are arguably more helpful; but for consistency I'd probably remove them too. (Or rather, don't add them.)
100% agree. Done


================
Comment at: pstl/include/pstl/internal/execution_impl.h:30-32
+__is_iterator_of(int) -> decltype(
+    std::conjunction<std::is_base_of<
+        _IteratorTag, typename std::iterator_traits<std::decay_t<_IteratorTypes>>::iterator_category>...>{});
----------------
Quuxplusone wrote:
> `decltype(T{})` is just `T`.
There reason of `decltype` was SFINAE. `iterator_category` is not always the part of `iterator_traits`. I thought it might be useful when we adopt take this approach from PSTL upstream to oneDPL.

On the other hand, we don't have much to do when the passed types by users are not iterators. So, yeah, let's keep it simple. Probably if we need additional logic in our code we can make it in other place.


================
Comment at: pstl/include/pstl/internal/execution_impl.h:38-41
+template <typename... _IteratorTypes>
+struct __is_random_access_iterator : decltype(__is_iterator_of<std::random_access_iterator_tag, _IteratorTypes...>(0))
+{
+};
----------------
rodgert wrote:
> Quuxplusone wrote:
> > Could this be:
> > ```
> > template <typename... _IteratorTypes>
> > using __are_random_access_iterators = decltype(__internal::__is_iterator_of<
> >     std::random_access_iterator_tag, _IteratorTypes...>(0));
> > ```
> > (Notice the ADL-proofing on `__is_iterator_of`.)
> > 
> > IIUC, the intent is that `__are_random_access_iterators<int*, char*>` is `true_type` but `__are_random_access_iterators<int*, std::list<int>::iterator>` is `false_type`, right?
> > 
> > Or if C++17 is permitted, then
> > ```
> > template <typename... _IteratorTypes>
> > using __are_random_access_iterators = std::bool_constant<
> >     (std::is_base_of_v<std::random_access_iterator_tag,
> >                        typename std::iterator_traits<_IteratorTypes>::iterator_category> && ...)
> > >;
> > ```
> At least as it relates to libstdc++'s consumption of pstl, C++17 is the baseline expectation.
>Or if C++17 is permitted, then

Sure, C++17 is permitted. `std::conjunction` is already C++17 API.

> IIUC, the intent is that `__are_random_access_iterators<int*, char*>` is `true_type` but `__are_random_access_iterators<int*, std::list<int>::iterator>` is `false_type`, right?

right.

I think I made it even simpler (at least from my point of view) and I would prefer to leave it as is if no strong objections. I also would prefer to have such a logic separation for `__are_random_access_iterators ` and `__are_iterators_of`


================
Comment at: pstl/include/pstl/internal/execution_impl.h:129-142
+template <class _IsVector>
+struct __serial_tag
+{
+    using __is_vector = _IsVector;
+};
+
+template <class _IsVector>
----------------
Quuxplusone wrote:
> IIUC, `__is_vector` is always `true_type` or `false_type`?
> Would it be simpler to define...
> ```
> template<bool _IsParallel, bool _IsVector>
> struct __bikeshed_tag {
>     using __is_vector = std::bool_constant<_IsVector>;
>     using __is_parallel = std::bool_constant<_IsParallel>;
>     using __backend_tag = std::conditional_t<_IsParallel, __par_backend_tag, void>;
> };
> 
> template<bool _IsVector>
> using __parallel_tag = __bikeshed_tag<true, _IsVector>;
> 
> template<bool _IsVector>
> using __serial_tag = __bikeshed_tag<false, _IsVector>;
> ```
> And then if you ever found any need to pattern-match on `__bikeshed_tag<X, true>`, it wouldn't be so awkward.
It probably would be simpler from searching standpoint but I don't think it's more correct comparing with what we have in the patch. `serial_tag` should not know about parallelism and backend. I don't tend to think that adding stubs (like `void`) is a good idea.

Probably I misunderstood the idea but I cannot find the applicability for `using __is_parallel = std::bool_constant<_IsParallel>;` line. It seems unnecessary to me because everything that we already have what we need to build the dispatch (I mean that `__parallel_tag` and `__serial_tag` are different types in your suggestion). Currently there is no need to read `__is_parallel` from somewhere.

What I do believe is we need to find better names for tags.

For backend tags I would propose `serial_backend_tag` and `tbb_backend_tag` instead of `serial_backend` and `tbb_backend` correspondently
For top level tags I would tend to think about `parallel_pattern_tag`, `serial_pattern_tag`, but probably "serial" is a bad word and "pattern" word makes the name too verbose. I am open for ideas 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104492