Skip to content
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

[SYCL] Make joint_reduce work with sub_group #8786

Merged
merged 2 commits into from
Mar 29, 2023

Conversation

jzc
Copy link
Contributor

@jzc jzc commented Mar 27, 2023

Note: the unqualified name lookup of joint_reduce in the overload of joint_reduce without an init param was not finding the overload of joint_reduce with an init param (because that declaration was located after it), so it searched for joint_reduce via ADL. With sycl::group, ADL can find both overloads of joint_reduce, but with sycl::sub_group = sycl::ext::oneapi::sub_group, ADL finds no joint_reduce in sycl::ext::oneapi.

Fixes #8348

@jzc jzc requested a review from a team as a code owner March 27, 2023 19:35
@jzc jzc requested a review from steffenlarsen March 27, 2023 19:35
@jzc jzc temporarily deployed to aws March 27, 2023 20:12 — with GitHub Actions Inactive
@jzc jzc temporarily deployed to aws March 27, 2023 21:46 — with GitHub Actions Inactive
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.

Good find! Could you please add a simple test to prove that it now works as intended? It may be enough to have a test that just ensures it compiles (sycl/test) but E2E testing arguably adds better coverage (sycl/test-e2e).

Note: the unqualified name lookup of joint_reduce in the overload of
joint_reduce without an init param was not finding the overload of
joint_reduce with an init param (because that declaration was located
after it), so it searched for joint_reduce via ADL. With sycl::group,
ADL can find both overloads of joint_reduce, but sycl::sub_group =
sycl::ext::oneapi::sub_group, ADL finds no joint_reduce in
sycl::ext::oneapi.

Signed-off-by: Cai, Justin <[email protected]>
@AlexeySachkov AlexeySachkov requested a review from a team March 28, 2023 15:27
@jzc jzc force-pushed the joint_reduce_sub_group branch from 8ef2edc to d716762 Compare March 28, 2023 16:17
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!

@jzc jzc temporarily deployed to aws March 28, 2023 17:35 — with GitHub Actions Inactive
@jzc jzc temporarily deployed to aws March 28, 2023 20:24 — with GitHub Actions Inactive
@bader bader merged commit cb91c23 into intel:sycl Mar 29, 2023
@steffenlarsen
Copy link
Contributor

steffenlarsen commented Mar 29, 2023

@jzc - It looks like GroupAlgorithm/reduce_sycl2020.cpp is failing in post-commit and pre-commit CI on L0 Gen9 and CUDA. Can you please investigate?

See post-commit for this PR and pre-commit results in #8709.

@jzc
Copy link
Contributor Author

jzc commented Mar 29, 2023

@jzc - It looks like GroupAlgorithm/reduce_sycl2020.cpp is failing in post-commit and pre-commit CI on L0 Gen9 and CUDA. Can you please investigate?

See post-commit for this PR and pre-commit results in #8709.

Yea, looking into it now.

bader pushed a commit that referenced this pull request Mar 29, 2023
Fixes changes made to `sycl_reduce2020.cpp` E2E test in #8786 - the
output array passed to `test` in the `std::complex<double>` case was too
small.

Signed-off-by: Cai, Justin <[email protected]>
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.

Compilation failure on sycl::joint_reduce with sub_group
3 participants