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.

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

SYCL TeamPolicy::team_scan #3815

Merged
merged 5 commits into from
Mar 18, 2021
Merged

SYCL TeamPolicy::team_scan #3815

merged 5 commits into from
Mar 18, 2021

Conversation

masterleinad
Copy link
Contributor

Based on #3783.

@masterleinad
Copy link
Contributor Author

Retest this please.

1 similar comment
@masterleinad
Copy link
Contributor Author

Retest this please.

@masterleinad masterleinad mentioned this pull request Mar 2, 2021
Merged
@masterleinad masterleinad marked this pull request as ready for review March 3, 2021 19:37
@masterleinad masterleinad requested a review from nliber March 3, 2021 19:37
@masterleinad
Copy link
Contributor Author

Retest this please.

@masterleinad masterleinad added this to the Tentative 3.4 Release milestone Mar 9, 2021
nliber
nliber requested changes Mar 9, 2021
View reviewed changes
core/src/SYCL/Kokkos_SYCL_Team.hpp Outdated Show resolved Hide resolved
core/src/SYCL/Kokkos_SYCL_Team.hpp Show resolved Hide resolved
@masterleinad masterleinad mentioned this pull request Mar 9, 2021
Open
@nliber nliber self-requested a review March 9, 2021 20:15
@masterleinad
Copy link
Contributor Author

There is a problem with global_accum that I will fix ASAP.

crtrott
crtrott reviewed Mar 10, 2021
View reviewed changes
core/src/SYCL/Kokkos_SYCL_Team.hpp Outdated
// FIXME_SYCL move somewhere else and combine with other places that do
// parallel_scan
// Exclusive scan returning the total sum, compare
// https://developer.nvidia.com/gpugems/gpugems3/part-vi-gpu-computing/chapter-39-parallel-prefix-sum-scan-cuda
Copy link
Member

Choose a reason for hiding this comment

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

We need to figure out what the license of this is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I can easily rewrite this to look more similar to what we have elsewhere if that's a concern.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I rewrote it to look more similar to RangePolicy::parallel_scan.

@masterleinad
Copy link
Contributor Author

Fixed the problem with global_accum, enabled team_reduction_scan, and rebased on top of #3818.

@masterleinad masterleinad added the Blocks Promotion Overview issue for release-blocking bugs label Mar 11, 2021
@masterleinad masterleinad mentioned this pull request Mar 11, 2021
Merged
dalg24
dalg24 reviewed Mar 15, 2021
View reviewed changes
Copy link
Member

@dalg24 dalg24 left a comment

Choose a reason for hiding this comment

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

Minor comments

core/src/SYCL/Kokkos_SYCL_Team.hpp Outdated Show resolved Hide resolved
example/tutorial/Hierarchical_Parallelism/CMakeLists.txt Outdated Show resolved Hide resolved
@masterleinad masterleinad mentioned this pull request Mar 16, 2021
Merged
8 tasks
@masterleinad
Copy link
Contributor Author

Rebased.

@dhollman dhollman self-requested a review March 17, 2021 20:15
dhollman
dhollman reviewed Mar 17, 2021
View reviewed changes
core/src/SYCL/Kokkos_SYCL_Parallel_Team.hpp
Comment on lines 444 to 446
m_shmem_begin = (sizeof(double) * (m_team_size + 2));
m_shmem_size =
(m_policy.scratch_size(0, m_team_size) +
Copy link

Choose a reason for hiding this comment

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

I know we haven't been reusing code, but if we're going to have something that's duplicated but very slightly different like this (from, e.g., Kokkos_Cuda_Parallel.hpp:790-795), can we at least add comments talking about what's different and why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you referring to m_shmem_sizeor something else? AFAICT, its initialization looks the same for SYCL and CUDA.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, you mean m_scratch_size[0] which is slightly different. It turns out that this variable is not used for Cuda but m_shmem_size instead. For SYCL we actually use m_scratch_size[0] instead of m_shmem_size in some places (of course they still have the same value). We could probably just replace the m_scratch_size C-array by a simple int having the value of the current m_scratch_size[1] in the related backends.

dhollman
dhollman approved these changes Mar 17, 2021
View reviewed changes
Copy link

@dhollman dhollman left a comment

Choose a reason for hiding this comment

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

LGTM

core/src/SYCL/Kokkos_SYCL_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 Outdated Show resolved Hide resolved
@crtrott crtrott merged commit 574f907 into kokkos:develop Mar 18, 2021
@masterleinad masterleinad deleted the sycl_team_scan branch March 18, 2021 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dalg24 dalg24 dalg24 left review comments

@crtrott crtrott crtrott left review comments

@dhollman dhollman dhollman approved these changes

@nliber nliber nliber approved these changes

Assignees
No one assigned
Labels
Blocks Promotion Overview issue for release-blocking bugs
Projects
None yet
Milestone
Tentative 3.4 Release
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@masterleinad @dhollman @dalg24 @nliber @crtrott