-
Notifications
You must be signed in to change notification settings - Fork 787
[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
[NFCI][SYCL] Refactor reductions implementations #7322
Conversation
7156403
to
918126d
Compare
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.
918126d
to
82dbc8a
Compare
sycl/include/sycl/reduction_fwd.hpp
Outdated
@@ -0,0 +1,67 @@ | |||
#pragma once |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this 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.
Failures:
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? |
@aelovikov-intel, please find the cause of fails. @againull please don't merge until cause is presented and owner identified. |
Got it, sorry. |
#7345 should hopefully address those warnings - my local build doesn't have them for some reason. |
SYCL :: Regression/device_num.cpp - Reported to owner in intel/llvm-test-suite#1354 (comment) with fix up in #7336. |
The purpose of this change is
Make it clear that sycl::range version always delegates to some
sycl::nd_range implementation.
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.
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
reduction_fordward.hpp
not logic updates.