Re-implement SYCL backend parallel_for to improve bandwidth utilization

[mmichel11]

High Level Description
This PR improves hardware bandwidth utilization of oneDPL's SYCL backend parallel for pattern through two ideas:

• Process multiple input iterations per work-item which involves a switch to a nd_range kernel combined with a sub / work group strided indexing approach.
• To generate wide loads for small types, implement a path that vectorizes loads / stores by processing adjacent indices within a single work item. This is combined with the above approach to maximize hardware bandwidth utilization. Vectorization is only applied to fundamental types of size less than 4 (e.g. uint16_t, uint8_t) under a contiguous container.

Implementation Details

• Parallel for bricks have been reworked in the following manner:
  • Each brick contains a pack of ranges within its template parameters to define tuning parameters.
  • The following static integral members are defined (implemented with inheritance):
    • __can_vectorize
    • __preferred_vector_size (1 if __can_vectorize is false)
    • __preferred_iters_per_item
  • The following public member functions are defined
    • __scalar_path (for small input sizes this member function is explicitly called)
    • __vector_path (optional for algorithms that are not vectorizable e.g. binary_search)
    • An overloaded function call operator which dispatches to the appropriate strategy

To implement this approach, the parallel for kernel rewrite from #1870 was adopted with additional changes to handle vectorization paths. Additionally, generic vectorization and strided loop utilities have been defined with the intention for these to be applicable in other portions of the codebase as well. Tests have been expanded to ensure coverage of vectorization paths.

This PR will supersedes #1870. Initially, the plan was to merge this PR into 1870 but after comparing the diff, I believe the most straightforward approach will be to target this directly to main.

[mmichel11]

So now we have 3 entity with defined constexpr static bool __can_vectorize :

1. class walk_vector_or_scalar_base
2. class walk_scalar_base
3. struct __brick_shift_left

Does these constexpr-variables really has different semantic?

And if the semantic of these entities are the same, may be make sense to make some re-design to have only one entity __can_vectorize ?

These three cases are all unique when you consider that they define __can_vectorize, __preferred_vector_size, and __preferred_iters_per_item. These three fields are all tightly coupled, so in my opinion it makes sense to define them together for readability. If we were to define a single __can_vectorize, then I think it would need to function more like a trait class dependent on the provided brick as the brick itself plays a roll in whether not vectorization is possible. This design would not get us much in my opinion as we would still need specializations for the different cases.

The three unique cases I mention are the following:

1. struct walk_vector_or_scalar_base - Vectorization is possible so long as the ranges meet the requirements to be vectorizable. This is then used to determine iters per item and the vector size.
2. struct walk_scalar_base - Vectorization is not possible due to some limitation of the brick. Binary search is a good example since its accesses are non-sequential. The iterations per work item is still set based on the size of the provided ranges.
3. struct __brick_shift_left - This brick has a limitation that prevents vectorization and that only one iteration per item can be processed and is a special case.

[mmichel11]

In some moments implementation details remind me tag-dispatching which were designed by @rarutyun. But with some differences: for example the walk2_vectors_or_scalars has not only information about vectorization or parallelization should be executed, but also two variant of functional staff and operator() with compile-time condition check to run one code or another code.

But what if we instead of two different functions

    template <typename _IsFull, typename _ItemId>
    void
    __vector_path(_IsFull __is_full, const _ItemId __idx, _Range __rng) const
    {
        // This is needed to enable vectorization
        auto __raw_ptr = __rng.begin();
        oneapi::dpl::__par_backend_hetero::__vector_walk<__base_t::__preferred_vector_size>{__n}(__is_full, __idx, __f,
                                                                                                 __raw_ptr);
    }

    // _IsFull is ignored here. We assume that boundary checking has been already performed for this index.
    template <typename _IsFull, typename _ItemId>
    void
    __scalar_path(_IsFull, const _ItemId __idx, _Range __rng) const
    {

        __f(__rng[__idx]);
    }

we will have some two functions with the same name and the format excepting the first parameter type which will be used as some tag ?

Please take a look at __parallel_policy_tag_selector_t for details.

Thanks for the suggestion. I had previously considered this approach by having two function call operators and using
a true_type / false_type as a tag:

     template <typename _IsFull, typename _ItemId>
     void
     operator()(/*__vectorize*/std::true_type, _IsFull __is_full, const _ItemId __idx, _Range __rng) const
     {
         oneapi::dpl::__par_backend_hetero::__vector_walk<__base_t::__preferred_vector_size>{__n}(__is_full, __idx, __f,__rng);
     }
 
     // _IsFull is ignored here. We assume that boundary checking has been already performed for this index.
     template <typename _IsFull, typename _ItemId>
     void
     operator()(/*__vectorize*/std::false_type, _IsFull, const _ItemId __idx, _Range __rng) const
     {
         __f(__rng[__idx]);
     }

This allows us to remove the if constexpr ... else dispatch that shows up in each brick. The reason I decided against this is that the caller of the brick, oneapi::dpl::__par_backend_hetero::__strided_loop, would also need to have an additional vectorization tag that would need to be passed through to the brick. This is intended to be a generic utility, and I do not believe that we should require the caller of __strided_loop to provide any additional tag beyond the necessary _IsFull to avoid OOB accesses.

Alternatively, we could replace __scalar_path_impl / __vector_path_impl with a single function __impl that accepts a vector tag as its first parameter. This is less clear in my opinion compared to separate function names.

[SergeyKopienko]

I believe that we may try to improve this code by replacing run-time bool value use_32bit_indexing to compile-time indexing type specialization.
I found only 3 places with the code

const bool use_32bit_indexing = size <= std::numeric_limits<std::uint32_t>::max();

so it's not big deal to add if statement outside and call __parallel_for inside for both branches with the different index types. But inside the brick we exclude condition check at all.

[mmichel11]

As discussed offline, I will reevaluate performance here and provide an update. The advantage of the current approach is that we only compile a single kernel whereas your suggestion may improve kernel performance with the cost of increased JIT overhead.

[SergeyKopienko]

Should we really extract this into local variable? Will we have some profit?
We still have a lot of code without this local variable...

[mmichel11]

Initially, I did this to remove the duplicate all_view()s that arise but this actually seems to lead to more code looking at the diff. All of these added local view variables have been reverted in the PR.