Skip to content

[NFCI][SYCL] Refactor reductions implementations #7322

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 3 commits into from
Nov 10, 2022

Conversation

aelovikov-intel
Copy link
Contributor

@aelovikov-intel aelovikov-intel commented Nov 8, 2022

The purpose of this change is

  1. Make it clear that sycl::range version always delegates to some
    sycl::nd_range implementation.

  2. Flatten all existing implementations. That required splitting
    reduction_parallel_for_basic_impl depending on
    Reduction::has_fast_reduce. However, I also inlined its helper
    routines (that now had unambigious branches for "if
    constexpr (Reduction::*)") so the ratio between duplicate/unique code
    isn't bad at all.

  3. Make a unique dispatching entry and have all the implementations
    provide the same interface. I plan to use it in unit-tests to bypass
    the dispatch and test all the implementation directly (when
    applicable based on nd_range/BinOp/HW/etc.).

I'd say 90% of the change is straightforward code movement with the
exceptions of

  • Inlining described above
  • Simplifying __sycl_reduction_kernel helper enabled by this change
  • Factoring out forward declarations in handler.hpp into a separate
    reduction_fordward.hpp
  • Rewriting dispatcher routine - caused by changes in the interfaces,
    not logic updates.

@aelovikov-intel aelovikov-intel requested a review from a team as a code owner November 8, 2022 21:17
@aelovikov-intel aelovikov-intel force-pushed the reduction-refactor-single branch 2 times, most recently from 7156403 to 918126d Compare November 8, 2022 21:59
@aelovikov-intel aelovikov-intel changed the title [NFCI][SYCL][Reductions] Refactor single-reduction implementations [NFCI][SYCL] Refactor reductions implementations Nov 8, 2022
The purpose of this change is

1) Make it clear that sycl::range version always delegates to some
   sycl::nd_range implementation.

2) Flatten all existing implementations. That required splitting
   reduction_parallel_for_basic_impl depending on
   Reduction::has_fast_reduce. However, I also inlined its helper
   routines (that now had unambigious branches for "if
   constexpr (Reduction::*)") so the ratio between duplicate/unique code
   isn't bad at all.

3) Make a unique dispatching entry and have all the implementations
   provide the same interface. I plan to use it in unit-tests to bypass
   the dispatch and test all the implementation directly (when
   applicable based on nd_range/BinOp/HW/etc.).

I'd say 90% of the change is straightforward code movement with the
exceptions of

  - Inlining described above
  - Simplifying __sycl_reduction_kernel helper enabled by this change
  - Factoring out forward declarations in handler.hpp into a separate
    reduction_fwd.hpp
  - Rewriting dispatcher routine - caused by changes in the interfaces,
    not logic updates.
@aelovikov-intel aelovikov-intel force-pushed the reduction-refactor-single branch from 918126d to 82dbc8a Compare November 8, 2022 22:21
@@ -0,0 +1,67 @@
#pragma once
Copy link
Contributor

Choose a reason for hiding this comment

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

// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception

License header is missing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

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.

LGTM. Just a minor nit.

@aelovikov-intel
Copy link
Contributor Author

Failures:

SYCL :: Regression/device_num.cpp
SYCL :: regression/tanh_fix_test.cpp  / on ESIMD

seem to be unrelated (e.g., the same are reported on https://github.com/intel/llvm/actions/runs/3430654188/jobs/5718451880). @intel/llvm-gatekeepers can this be merged in?

@againull againull merged commit 7d0451c into intel:sycl Nov 10, 2022
@pvchupin
Copy link
Contributor

@aelovikov-intel, please find the cause of fails. @againull please don't merge until cause is presented and owner identified.
Also there are post-commit fails: https://github.com/intel/llvm/actions/runs/3432868067

@againull
Copy link
Contributor

@aelovikov-intel, please find the cause of fails. @againull please don't merge until cause is presented and owner identified. Also there are post-commit fails: https://github.com/intel/llvm/actions/runs/3432868067

Got it, sorry.

@aelovikov-intel
Copy link
Contributor Author

Also there are post-commit fails: https://github.com/intel/llvm/actions/runs/3432868067

#7345 should hopefully address those warnings - my local build doesn't have them for some reason.

@aelovikov-intel aelovikov-intel deleted the reduction-refactor-single branch November 10, 2022 07:01
@steffenlarsen
Copy link
Contributor

SYCL :: Regression/device_num.cpp - Reported to owner in intel/llvm-test-suite#1354 (comment) with fix up in #7336.
SYCL :: regression/tanh_fix_test.cpp - Reported to owner in intel/llvm-test-suite#1361 (comment) with fix in intel/llvm-test-suite#1372.

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.

5 participants