dual view: update template types + Kokkos::is_dual_view (#6085)

[romintomasetti]

• This PR closes Kokkos::DualView is still on <DataType, class, class, class> instead of <DataType, Properties...> #6085.
• It introduces Kokkos::is_dual_view trait and Kokkos::is_dual_view_v helper variable template for consistency with other Kokkos containers.

[dalg24-jenkins]

Can one of the admins verify this patch?

[romintomasetti]

@ajpowelsnl There were a little more changes required...

[ajpowelsnl]

@ajpowelsnl There were a little more changes required...

Please let us know if there's anything we can do to help ...

[romintomasetti]

@ajpowelsnl I think this is ready for your approval to test 😄

[dalg24]

(peer reviewed with Daniel)

[dalg24]

Comment about that change.

[romintomasetti]

I'm not sure if you mean commenting that in the code or in the PR...

I'll comment it here first.

The whole point of this PR is to allow a Kokkos::DualView to accept any Properties... that a Kokkos::View can accept. Before this PR, it was hardcoded that DualView only accepts 4 types:

1. DataType
2. Arg1Type
3. Arg2Type
4. Arg3Type

but there was no check enforcing the type of the last 3 (Arg1Type, Arg2Type, Arg3Type). So they could be anything that Kokkos::ViewTraits would accept. However, DualViewSubview::type would then "hardcode" that when taking a subview of a dual view, only data type, array layout, device type and memory traits would be set. If my understanding is correct, that could have lead to issues if for instance I had created my original dual view with some hooks policy and then took a subview of it because my subview would have the default hooks policy (please correct me if this isn't correct! 😉).

However, I must admit that I'm not satisfied of my solution because I only added hooks_policy while Kokkos::ViewTraits may also accept specialize and others.

So my question for you @dalg24 is how I can ensure that all traits are passed to DualViewSubviewType::type? I guess ideally one should be able to do:

template <class DV, class... Args>
struct DualViewSubviewType {
  static_assert(Kokkos::is_dual_view_v<DV>);

  using dst_traits = typename Kokkos::Impl::ViewMapping<
      void, typename DV::traits, Args...>::traits_type;

  using type = Kokkos::DualView<dst_traits>;
};

where dst_traits is a Kokkos::ViewTraits that would then embed all the traits given by the user and this would ensure that all types are passed. But I agree this would not work so I wonder how one can ensure that DualViewSubviewType::type will always "forward" all types, even in the future when ViewTraits would accept some more types...

[dalg24]

I would have tried something like

namespace Impl {

template <class V>
struct V2DV;

template <class D, class... P>
struct V2DV<View<D, P...>> {
  using type = DualView<D, P...>;
};
} /* namespace Impl */

template <class DataType, class... Properties, class... Args>
auto subview(const DualView<DataType, Properties...>& src, Args... args) {
  // leverage Kokkos::View facilities to deduce the properties of the subview
  using deduce_subview_type =
      decltype(subview(std::declval<View<DataType, Properties...>>(), args...));
  // map it back to dual view
  return typename Impl::V2DV<deduce_subview_type>::type(src, args...);
}

to defer the deduction to Kokkos Core.
Ping @nmm0 who introduced the view hooks and @masterleinad who recently refactored subview(View<...>, ...)

I grepped for "subview" in the containers unit test and the test coverage looks pretty light.

[romintomasetti]

@dalg24 This is indeed a much nicer solution!

[romintomasetti]

I'm still wondering why these args... are never perfect-forwarded though... (usually they are integer values but they could be e.g. pairs).

[dalg24]

You can use perfect forwarding if you like.

[dalg24]

Comment about why this is ok to delete that comment and not trust whoever wrote that.

[romintomasetti]

Well, my first feeling was that this code was wrong because it was calling modify_host/modify_device on a (shallow) copy of dst. But as the modified_flags member is a (shared) view, it seems this is still OK.

Still I was not "trusting" such a comment like trust me, this must not be a reference (reminds me of "Trust me I'm an engineer") because to me this should work with a reference otherwise there must be other cases where taking a dual view by reference is dangerous.

So I wanted to put my hypothesis that in fact it works with a reference to the test.

On my side, it worked for OpenMP and Cuda (without UVM).

[dalg24]

I am leaning towards saying your change is fine.
My point was this needs to be discussed and not swept under the rug. It looks like the comment has been there since the initial commit in this project when the code was extracted from Sandia's Trilinos repository.
cc @crtrott

[masterleinad]

It was introduced in trilinos/Trilinos@0806efc by @mhoemmen.

[masterleinad]

Looks good to me given that we discuss changing the copy in deep_copy to a reference some.

[dalg24]

I am ok with changing to a reference in deep_copy

[dalg24]

OK to test

[masterleinad]

I would prefer

Suggested change

	DualView<DT, DP...>& dst,
	const DualView<DT, DP...>& dst,

to mirror the overloads for Kokkos::View more closely.

[romintomasetti]

Done 😉

[romintomasetti]

I'm reverting this because these functions are calling modify_host on dst that mustn't be const:

In file included from /__w/kokkos/kokkos/containers/unit_tests/TestDualView.hpp:25:
/__w/kokkos/kokkos/containers/unit_tests/../src/Kokkos_DualView.hpp:1194:9: error: no matching member function for call to 'modify_host'
    dst.modify_host();
    ~~~~^~~~~~~~~~~
/__w/kokkos/kokkos/containers/unit_tests/TestDualView.hpp:185:13: note: in instantiation of function template specialization 'Kokkos::deep_copy<int **, Kokkos::LayoutLeft, Kokkos::OpenMP, int **, Kokkos::LayoutLeft, Kokkos::OpenMP>' requested here
    Kokkos::deep_copy(b, a);

[romintomasetti]

Ready to go 🚀