Skip to content

[NFCI][SYCL] Refactor reduction implementation #6445

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

Merged
merged 24 commits into from
Jul 27, 2022

Conversation

aelovikov-intel
Copy link
Contributor

@aelovikov-intel aelovikov-intel commented Jul 15, 2022

The core part of it is adding the user's reduction variable type as an
explicit template parameter and modifying reduction to store just that,
instead of having three members (RWAcc/DWAcc/UsmPtr) of which only one
is used to store it.

That, in turn, made the whole default_reduction_algorithm redundant as
all it was used for was to propagate that exact information in an
obscure way.

Once done, it enabled some further simplifications:

  • Simplifying some methods to use compile-time "if constexpr" instead
    of checking the type of the user reduction variable in runtime
    (i.e., previous "if (MRWAcc)" check).

  • Making reduction_impl_algo::reduction_impl_algo ctor generic and
    having a single instance of it, eliminating three version for
    different types of user's reduction variable.

  • Unify reduction_impl::reduction_impl ctors accessing RW/DW accessor
    eliminating code duplication

  • create make_reduction helper function to make creation of
    reduction_impl require less boilerplate code by using template type
    parameters deduction (couldn't use CTAD as some of the params can't
    be deduced and there is no such thing as partial CTAD).

  • Simplify reduction_impl creation further by benefiting from
    accessor's CTAD.

@aelovikov-intel aelovikov-intel changed the title [WIP][NFCI][SYCL] Refactor reduction implementation [NFCI][SYCL] Refactor reduction implementation Jul 15, 2022
@aelovikov-intel aelovikov-intel marked this pull request as ready for review July 15, 2022 18:32
@aelovikov-intel aelovikov-intel requested a review from a team as a code owner July 15, 2022 18:32
@aelovikov-intel aelovikov-intel force-pushed the reduction2 branch 2 times, most recently from 9d2af67 to 8d527c8 Compare July 18, 2022 23:51
@aelovikov-intel
Copy link
Contributor Author

Failures are unrelated, I believe:

Failed Tests (4):
SYCL :: dpas/dpas_test1.cpp
SYCL :: dpas/dpas_test2.cpp
SYCL :: dpas/dpas_test3.cpp
SYCL :: unused_spec_const.cpp

steffenlarsen
steffenlarsen previously approved these changes Jul 27, 2022
Copy link
Contributor

@steffenlarsen steffenlarsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comment, but otherwise LGTM.

@aelovikov-intel
Copy link
Contributor Author

The failures are unrelated to this PR, I believe:

Failed Tests (4):
SYCL :: dpas/dpas_test1.cpp
SYCL :: dpas/dpas_test2.cpp
SYCL :: dpas/dpas_test3.cpp
SYCL :: unused_spec_const.cpp

Hence, @intel/llvm-gatekeepers this PR is ready.

@pvchupin
Copy link
Contributor

Fails are getting addressed at intel/llvm-test-suite#1115 and intel/llvm-test-suite#1117

@pvchupin pvchupin merged commit 92678f0 into intel:sycl Jul 27, 2022
@aelovikov-intel aelovikov-intel deleted the reduction2 branch August 25, 2022 20:10
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.

3 participants