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 TeamPolicy nested parallel_reduce #3783

Merged
merged 3 commits into from
Mar 3, 2021

Conversation

masterleinad
Copy link
Contributor

Based on top of #3763, this pull request implements TeamPolicy parallel_reduce. Currently, vector_length=1 is assumed.

@masterleinad masterleinad marked this pull request as draft February 8, 2021 14:41
@masterleinad masterleinad force-pushed the sycl_team_reduce branch 2 times, most recently from 8d9bc1c to 0a27d7e Compare February 8, 2021 17:39
@masterleinad masterleinad marked this pull request as ready for review February 11, 2021 22:53
@masterleinad masterleinad marked this pull request as draft February 11, 2021 23:08
@masterleinad masterleinad marked this pull request as ready for review February 12, 2021 16:15
@masterleinad masterleinad changed the title SYCL TeamPolicy parallel_reduce SYCL TeamPolicy nested parallel_reduce Feb 18, 2021
@masterleinad masterleinad marked this pull request as draft February 19, 2021 03:43
@masterleinad
Copy link
Contributor Author

masterleinad commented Feb 19, 2021

This should work now. team_reduce should be (re-)reviewed.

@masterleinad masterleinad marked this pull request as ready for review February 19, 2021 17:38
core/unit_test/TestTeamVector.hpp Show resolved Hide resolved
core/src/Kokkos_SYCL_Space.hpp Show resolved Hide resolved
core/src/SYCL/Kokkos_SYCL_Space.cpp Outdated Show resolved Hide resolved
core/src/SYCL/Kokkos_SYCL_Space.cpp Outdated Show resolved Hide resolved
core/src/SYCL/Kokkos_SYCL_Team.hpp Outdated Show resolved Hide resolved
core/src/SYCL/Kokkos_SYCL_Team.hpp Outdated Show resolved Hide resolved
core/src/SYCL/Kokkos_SYCL_Parallel_Team.hpp Outdated Show resolved Hide resolved
@masterleinad
Copy link
Contributor Author

@dalg24 I addressed your comments.

@dalg24
Copy link
Member

dalg24 commented Mar 1, 2021

What is the plan for relaxing the requirement for the vector length to be 1?

@masterleinad
Copy link
Contributor Author

What is the plan for relaxing the requirement for the vector length to be 1?

My current plan is to have all the functionality working for vector length == 1 and then to tackle larger sizes. Conceptionally, this should not be a problem.

core/src/Kokkos_SYCL_Space.hpp Outdated Show resolved Hide resolved
core/src/SYCL/Kokkos_SYCL_Team.hpp Outdated Show resolved Hide resolved
Copy link
Member

@crtrott crtrott left a comment

Choose a reason for hiding this comment

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

Left some comments regarding team_size/vector_lemgth folding in one dim

core/src/SYCL/Kokkos_SYCL_Parallel_Team.hpp Outdated Show resolved Hide resolved
core/src/SYCL/Kokkos_SYCL_Team.hpp Show resolved Hide resolved
core/src/SYCL/Kokkos_SYCL_Team.hpp Show resolved Hide resolved
core/src/SYCL/Kokkos_SYCL_Team.hpp Show resolved Hide resolved
core/src/SYCL/Kokkos_SYCL_Team.hpp Show resolved Hide resolved
Copy link
Member

@crtrott crtrott left a comment

Choose a reason for hiding this comment

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

looks good to me now.

@masterleinad
Copy link
Contributor Author

Retest this please.

@@ -493,6 +584,7 @@ KOKKOS_INLINE_FUNCTION void parallel_for(
const Impl::ThreadVectorRangeBoundariesStruct<iType, Impl::SYCLTeamMember>&
loop_boundaries,
const Closure& closure) {
// FIXME_SYC: adapt for vector_length!=1
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be FIXME_SYCL (so we don't miss it later).

@crtrott crtrott merged commit 3ed71ed into kokkos:develop Mar 3, 2021
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.

None yet

4 participants