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

Small change to parallel_reduce fence #3359

Merged
merged 1 commit into from
Sep 10, 2020

Conversation

dhollman
Copy link

@dhollman dhollman commented Sep 8, 2020

This doesn't change any functionality, but it's a refactoring that makes it possible to do some of the necessary checking in the Kokkos Graphs interface, since anything that needs a fence there is not allowed.

This doesn't change any functionality, but it's a refactoring that makes
it possible to do some of the necessary checking in the Kokkos Graphs
interface, since anything that needs a fence there is not allowed.
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.

LGTM overall. You have my approval after the "view-like" question is cleared up.

template <class ExecutionSpace, class ViewLike>
constexpr std::enable_if_t<
// requires Kokkos::ViewLike<ViewLike>
Kokkos::is_view<ViewLike>::value,
Copy link
Member

Choose a reason for hiding this comment

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

I don't get the "view-like". Are you referring to some upcoming changes?

Copy link
Author

Choose a reason for hiding this comment

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

I didn't want to use the token View because it shadows something in namespace Kokkos. But more to the point: in theory, things like DynRankView should also meet the requirements of ViewLike as a concept. Anyway, it's just a template parameter—don't read too much into it; I've just always thought of the concept for View as being named ViewLike, but do you have a better name for that concept?

@dalg24 dalg24 merged commit fc1cfe4 into kokkos:develop Sep 10, 2020
@dhollman dhollman deleted the parallel-reduce-needs-fence-change branch September 21, 2020 22:46
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

2 participants