Skip to content

Remove use of inheritance from parallel-for bricks in SYCL backend #2136

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 19 commits into from

Conversation

mmichel11
Copy link
Contributor

@mmichel11 mmichel11 commented Mar 18, 2025

This PR addresses #2023.

In #1976, parallel-for bricks were redesigned to have an inheritance relationship with a either a scalar or vector tuning base class in order to inherit compile-time tuning constants. This came with a downside of adding the user-provided range types to the class template of parallel-for bricks, complicating usage from the caller side as more templates must be provided when creating a brick.

This PR removes the use of inheritance from parallel-for bricks along with the range class templates with the following approach:

  • Each parallel-for brick defines two compile-time boolean constants, __can_vectorize and __can_process_multiple_iters. __can_vectorize defines whether a vectorization path is provided. For some bricks (e.g. __custom_brick for binary search) there is no benefit of providing a vectorization path. __can_process_multiple_iters specifies whether a single item can safely process multiple strided iterations of input.
  • The parallel-for SYCL implementation makes use of a new class __pfor_params which queries the brick's static members to define vectorization and iters per item tuning parameters.
  • An instance of __pfor_params is passed to the brick when invoking the function call operator which includes vector sizes and whether vectorization should be enabled.

Additionally, the __scalar_path_impl, __vector_path_impl, and dispatch operator() design of the bricks have been replaced with two operator() definitions using SFINAE to define scalar and vector implementations dependent on whether the provided parameter instance is vectorizable.

@SergeyKopienko
Copy link
Contributor

@mmichel11 Does we really have some benefits of using std::enable_if_t<_Params::__b_vectorize in 9 places in comparison if constexpr ?

@mmichel11
Copy link
Contributor Author

@mmichel11 Does we really have some benefits of using std::enable_if_t<_Params::__b_vectorize in 9 places in comparison if constexpr ?

I will take a look at using if constexpr instead and see if that is cleaner than the current approach. For the brick's with a small function body, it should be.

Signed-off-by: Matthew Michel <[email protected]>
Signed-off-by: Matthew Michel <[email protected]>
Signed-off-by: Matthew Michel <[email protected]>
Signed-off-by: Matthew Michel <[email protected]>
Signed-off-by: Matthew Michel <[email protected]>
Signed-off-by: Matthew Michel <[email protected]>
@mmichel11 mmichel11 force-pushed the dev/mmichel11/pfor_remove_ranges_class_template branch from 96c8dcd to 27e0932 Compare April 14, 2025 00:28
Signed-off-by: Matthew Michel <[email protected]>
@mmichel11 mmichel11 marked this pull request as draft April 16, 2025 01:00
@mmichel11
Copy link
Contributor Author

Moving this to draft with the plan to close and do all review in PR #2165 unless there are objections. It prevents us from making changes in this PR that will just be removed in the subsequent.

@mmichel11
Copy link
Contributor Author

Closing, as this PR is no longer needed.

@mmichel11 mmichel11 closed this Apr 17, 2025
@mmichel11 mmichel11 deleted the dev/mmichel11/pfor_remove_ranges_class_template branch May 19, 2025 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants