Skip to content

[SYCL][Reduction] Use "if constexpr" over SFINAE #6343

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 11 commits into from
Jul 7, 2022
Merged

[SYCL][Reduction] Use "if constexpr" over SFINAE #6343

merged 11 commits into from
Jul 7, 2022

Conversation

aelovikov-intel
Copy link
Contributor

Several reasons:

  • The code is easier to read/understand.
  • Ability to mix compiler/run-time conditions in a single instance of logic
    description, including those in future (e.g. discrete vs integrated GPU).
  • Eliminate duplication of similar parts of different algorithms so that
    distinctions are better highlighted.
  • Ease of experiments when trying to switch algorithm used by decoupling hard
    requirements from the peformance tuning logic (i.e., calling slower code
    must not result in a compile-time error).
  • Allows to stop encoding implementation selection logic into overloads, use
    distinct names instead (e.g., see forward declarations in handler.hpp).

Drawbacks:

  • The feature is only enabled in C++17 mode now. The customers who need C++14
    mode aren't expected to require it.

    Mitigation plan would be to change remaining SFINAE logic inside reduction.hpp
    into runtime asserts and have "constexpr" changed to a compile-time macro
    __SYCL_REDUCTION_CONSTEXPR to resort to run-time only code selection in
    C++14 mode.

@aelovikov-intel
Copy link
Contributor Author

Just a prototype at this point to get feedback on the direction.

@aelovikov-intel
[SYCL][Reduction] Use "if constexpr" over SFINAE
ac3fd7c 
Several reasons:

  * The code is easier to read/understand.
  * Ability to mix compiler/run-time conditions in a single instance of logic
    description, including those in future (e.g. discrete vs integrated GPU).
  * Eliminate duplication of similar parts of different algorithms so that
    distinctions are better highlighted.
  * Ease of experiments when trying to switch algorithm used by decoupling hard
    requirements from the peformance tuning logic (i.e., calling slower code
    must not result in a compile-time error).
  * Allows to stop encoding implementation selection logic into overloads, use
    distinct names instead (e.g., see forward declarations in handler.hpp).

Drawbacks:
  * The feature is only enabled in C++17 mode now. The customers who need C++14
    mode aren't expected to require it.

    Mitigation plan would be to change remaining SFINAE logic inside reduction.hpp
    into runtime asserts and have "constexpr" changed to a compile-time macro
    __SYCL_REDUCTION_CONSTEXPR to resort to run-time only code selection in
    C++14 mode.
@steffenlarsen steffenlarsen self-requested a review June 23, 2022 09:34
v-klochkov
v-klochkov reviewed Jun 27, 2022
View reviewed changes
Copy link
Contributor

@v-klochkov v-klochkov left a comment

Choose a reason for hiding this comment

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

I have 3 quick comments.

sycl/include/CL/sycl/handler.hpp
}

template <class FunctorTy>
static event withAuxHandler(std::shared_ptr<detail::queue_impl> Queue,
Copy link
Contributor

Choose a reason for hiding this comment

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

This version is not used, if I am not missing anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Inside reduSaveFinalResultToUserMemHelper in reduction.hpp: https://github.com/intel/llvm/pull/6343/files#diff-6fee47fe508539a049886253dcc0f49dea9210ab0c1151feed4cdca1632f184dR2489

sycl/include/CL/sycl/handler.hpp Outdated
} // end while (NWorkItems > 1)

auto CopyEvent = ext::oneapi::detail::reduSaveFinalResultToUserMem(
QueueCopy, MIsHost, ReduTuple, ReduIndices);
if (CopyEvent)
MLastEvent = *CopyEvent;
}
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#endif
#endif // __cplusplus >= 201703L

@aelovikov-intel aelovikov-intel marked this pull request as ready for review June 30, 2022 00:03
@aelovikov-intel aelovikov-intel requested a review from a team as a code owner June 30, 2022 00:03
@aelovikov-intel
Copy link
Contributor Author

ping

steffenlarsen
steffenlarsen previously approved these changes Jul 5, 2022
View reviewed changes
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.

Only a handful of very minor comments. Great work!

steffenlarsen
steffenlarsen reviewed Jul 7, 2022
View reviewed changes
steffenlarsen
steffenlarsen approved these changes Jul 7, 2022
View reviewed changes
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!

@aelovikov-intel
Copy link
Contributor Author

Failing test looks unrelated and I think I've seen it in other PRs as well. @intel/llvm-gatekeepers , I believe this is ready.

@pvchupin
Copy link
Contributor

pvchupin commented Jul 7, 2022

@aelovikov-intel, similar issues have been addressed... do you have recent references to similar issue?

@aelovikov-intel
Copy link
Contributor Author

The flaky failure on the first pre-commit CI run has been recorded as #6416.

@pvchupin
Copy link
Contributor

pvchupin commented Jul 7, 2022

Thanks @aelovikov-intel for a follow up.

@pvchupin pvchupin merged commit 0cf7b45 into intel:sycl Jul 7, 2022
aelovikov-intel added a commit to aelovikov-intel/llvm that referenced this pull request Jul 20, 2022
@aelovikov-intel
[SYCL] Fix reductions regressions after intel#6343
db56558 
Three pieces:

  * Access to private static handler::withAuxHandler
  * Pow2WG -> HasUniformWG
  * Out accessor must be the last created (after other temps)
againull pushed a commit that referenced this pull request Jul 22, 2022
@aelovikov-intel
[SYCL] Fix reductions regressions after #6343 (#6460)
a0bfab1 
Three pieces:
  * Access to private static handler::withAuxHandler
  * Pow2WG -> HasUniformWG
  * Out accessor must be the last created (after other temps)
aelovikov-intel added a commit to aelovikov-intel/llvm that referenced this pull request Jul 29, 2022
@aelovikov-intel
[SYCL] Fix c++14 mode compilation after intel#6343
4b8ed60 
Some reduction code is used on non-reduction path and must be available
in C++14 even though the rest of the feature is guarded to be available
in C++17 and above only.
pvchupin pushed a commit that referenced this pull request Jul 29, 2022
@aelovikov-intel
[SYCL] Fix c++14 mode compilation after #6343 (#6497)
48414c7 
Some reduction code is used on non-reduction path and must be available
in C++14 even though the rest of the feature is guarded to be available
in C++17 and above only.
@aelovikov-intel aelovikov-intel deleted the reduction branch August 25, 2022 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@v-klochkov v-klochkov v-klochkov left review comments

@steffenlarsen steffenlarsen steffenlarsen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@aelovikov-intel @pvchupin @v-klochkov @steffenlarsen