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

Allow ViewCtorProp in create_mirror[_view] #5095

Merged
merged 15 commits into from
Jul 7, 2022

Conversation

masterleinad
Copy link
Contributor

Following-up on #4826, #4844, and #5035.

@PhilMiller
Copy link
Contributor

In support of #4792

@masterleinad masterleinad marked this pull request as ready for review June 15, 2022 16:45
@masterleinad
Copy link
Contributor Author

Most of the internals are now expressed in terms of a ViewCtorProp overload and there is also such a user-facing overload. In addition to current functionalities, this overload also allows specifying an execution space that avoids some fences depending on #4826.

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.

Couple questions but largely good. Follow up: can we get rid of some of the overlaods by making ViewCtorProps implicitly constructible from some of the explicit things.

auto mirror_host_view = Kokkos::create_mirror_view(
Kokkos::view_alloc(Kokkos::HostSpace{}, Kokkos::WithoutInitializing,
Kokkos::DefaultExecutionSpace{}),
host_view);
Copy link
Member

Choose a reason for hiding this comment

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

can we test the view_alloc(WithoutInitializing) too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, see 8c9aefb.

const I&... arg_prop) {
return Kokkos::create_mirror(arg_prop..., space, src);
const Impl::ViewCtorProp<ViewCtorArgs...>& arg_prop) {
return create_mirror(space, src, arg_prop);
Copy link
Member

Choose a reason for hiding this comment

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

did you want to call this here? Or rather add the space to the arg_prop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

}

// Create a mirror in a new space
template <class Space, class T, class... P>
inline auto create_mirror(
const Space& space, const Kokkos::Experimental::DynamicView<T, P...>& src) {
return Impl::create_mirror(space, src);
return Impl::create_mirror(space, src, Impl::ViewCtorProp<>{});
Copy link
Member

Choose a reason for hiding this comment

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

should we just pass view_alloc(space) in here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@masterleinad
Copy link
Contributor Author

Follow up: can we get rid of some of the overloads by making ViewCtorProps implicitly constructible from some of the explicit things.

No, I don't think that's possible. implicit conversions are not considered during type deduction, see https://en.cppreference.com/w/cpp/language/template_argument_deduction#Implicit_conversions. The only thing we could do is to derive from ViewCtorProps, see https://godbolt.org/z/48WcM9foa, but I don't think we should do that. In particular, not for execution spaces and memory spaces.

@crtrott crtrott added the Blocks Promotion Overview issue for release-blocking bugs label Jun 22, 2022
@crtrott crtrott merged commit 617b277 into kokkos:develop Jul 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Blocks Promotion Overview issue for release-blocking bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants