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.

[SergeyKopienko]

Why you pass __storage parameter by value?

[SergeyKopienko]

What is the case when __n < 0 is true?

[SergeyKopienko]

Why class walk_scalar_base declared as class but

template <typename _ExecutionPolicy, typename _F, typename _Range>
struct walk1_vector_or_scalar : public walk_vector_or_scalar_base<_Range>

declared as struct ?

[SergeyKopienko]

Empty string probably isn't required here.

[SergeyKopienko]

1. I think that __raw_ptr isn't very good name because begin() usually linked in mind with iterator. But raw usually is some pointer.
2. Do we really need to have here local variable __raw_ptr ? Can we pass __rng.begin() instead of that variable into __vector_walk call?

[SergeyKopienko]

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 ?

[SergeyKopienko]

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.

[SergeyKopienko]

Probably I don't understand something, but why this struct has name lazy?
It's looks like some kind of visitor pattern implementation, which call destroy() for each element in container.
What is exactly the lazy functional here?

[danhoeflinger]

I believe it is a callable deleter for __lazy_ctor_storage which is storage that has a delayed "lazy" constructor. Perhaps it would be better to instead add a static member function to the __lazy_ctor_storage union, get_deleter_callable(), which returns a lambda to delete a __lazy_ctor_storage& passed as an argument. This would remove any confusion, and group these together.

[SergeyKopienko]

We assume here that std::min(std::size_t{}, std::size_t{}) will always fit into std::uint8_t type?

[danhoeflinger]

I think it makes sense... __vec_size is 4 or less, but __n - __start_idx can only be assumed to fit within size_t (and you don't want to overflow before the min). The result will be 4 or less, which fits in 8 bits.

[SergeyKopienko]

One more point: __vector_path and __scalar_path tell me about some path but not imp.
May be better to rename them to ..._impl ?

[danhoeflinger]

First round of review. I've not gotten to all the details yet, but this is enough to be interesting.

[danhoeflinger]

I wonder if this formulation leaves us open for bugs in the future with no restrictions on what _WrapperType could be.
What we probably want is something like tuple-like from c++23.

Would we be better off limiting this to std::tuple and and onedpl's tuple with explicit partial specializations? or limit it via some enable_if magic?

Right now any templated type is reduced to its template arguments, which isn't always the case. Imagine a contrived user provided type for their input range which has a template argument which isn't used as a member field.

template <typename T>
struct __my_converting_type{
    std::uint8_t var;
    T get_conversion(){ return T{var};}
};

This would match the _WrapperType flavor I think, and return the wrong result if I understand the intention correctly. We would want such a type to use sizeof.

Godbolt link

[danhoeflinger]

I think we try to avoid letting testing specific code seep into the main repo, though I understand the need here to gain coverage.
Can we instead perhaps add one large test to the "normal" test suite which would hit the large submitter and enable it only under the same circumstances. I understand the desire to limit the test time of the suite, but this both infects the main repo with test specifics, but also adds coverage of this code in situations it will never encounter in the wild, and doesn't cover any real sizes.

Id really prefer not to, but if we do have to have this, I'd suggest uglifying the name.

[danhoeflinger]

Seems like we could move this out of the branch and use in both sides.

[danhoeflinger]

Is this a general utility which might have utility for other commutative operations beyond just parallel_for or is there a reason you believe this to be specific to this algorithm / kernel?

If we think it might be useful, we could lift this to a general utility level. Obviously we don't need to incorporate it elsewhere in this PR. An alternative is to add an issue to explore this and only lift it if we find utility.

[danhoeflinger]

I think it makes sense... __vec_size is 4 or less, but __n - __start_idx can only be assumed to fit within size_t (and you don't want to overflow before the min). The result will be 4 or less, which fits in 8 bits.

[danhoeflinger]

I really dislike having to pass {} as the first argument here. I'm not sure I even really understand why its necessary, is this for the base class?

Can we just define constructors which accepts only the brick and size to avoid this issue?

[danhoeflinger]

Lets fix the naming of this while were touching all its instances __custom_brick

[danhoeflinger]

Suggested change

[danhoeflinger]

I think it should be possible to combine walk*_vectors_or_scalars together with some complicated fold instructions, lambdas, tuples, and std::apply.

Take a look at the first answer of https://stackoverflow.com/questions/7230621/how-can-i-iterate-over-a-packed-variadic-template-argument-list. I think you should do something similar, chaining together instructions by returning tuples and then with std::apply.

Here is an example I was playing with.
https://godbolt.org/z/vc8dK4ed6

In the end, I'm not sure if (1) its actually possible and (2) its worth the complexity to consolidate these structs, but its worth considering...