DualView.template view() better matches for Devices in UVMSpace cases

[DavidPoliakoff]

Attempt to resolve: #3850 . My description is basically mimicking that, I recommend reading the bug there (h/t @brian-kelley )

In the past, we looked at whether a passed Device's memory space matched the memory space of t_dev (the device View), and returned something of the t_dev type if it did. This breaks in the case of

my_dual_view.view<Kokkos::Device<Kokkos::Serial, Kokkos::UVMSpace>()

The device's memory space matches t_dev's memory space, so we merrily return a View whose device is <Cuda,UVMSpace>. My PR adds checks for exact Kokkos::Device matches, and returns the appropriate one if a Device is passed. More info later if you need it, but that's what this does

Emphasizing this comment as loudly as I can: a Kokkos-side reviewer should think about whether this is an acceptable change. I think every change we've made here makes things more correct. But they are changes, and if somebody relied on our broken behavior, they might see differences and not be happy about them

edit: additional fix (necessary for tests to pass): modify functions now do nothing when there is only one device (note: not only one view)

[brian-kelley]

@DavidPoliakoff Can you try building my #3850 reproducer/example with this PR? I am getting a segfault with it at the dv.view<h_device>() that doesn't happen if I do dv.view_host(). It doesn't make sense because the backtrace looks like this:

#0  0x0000000000409dc3 in SharedAllocationTracker (enable_tracking=<optimized out>, rhs=..., this=0x7fffffffd6e0)
    at /home/bmkelle/KK_Clean/build/kokkos-install/include/impl/Kokkos_SharedAlloc.hpp:524
#1  Kokkos::Impl::ViewTracker<Kokkos::View<double*, Kokkos::LayoutLeft, Kokkos::Device<Kokkos::Serial, Kokkos::CudaUVMSpace> > >::ViewTracker (
    this=0x7fffffffd6e0, vt=...) at /home/bmkelle/KK_Clean/build/kokkos-install/include/impl/Kokkos_ViewTracker.hpp:77
#2  0x0000000000409581 in Kokkos::View<double*, Kokkos::LayoutLeft, Kokkos::Device<Kokkos::Serial, Kokkos::CudaUVMSpace> >::View (this=0x7fffffffd6e0)
    at /home/bmkelle/KK_Clean/build/kokkos-install/include/Kokkos_View.hpp:1426
#3  0x0000000000406fde in main () at /ascldap/users/bmkelle/KK_Clean/kokkos-kernels/perf_test/sparse/bmk.cpp:52

bmk.cpp:52 is auto v_h = dv.template view<h_device>(); where h_device is Device<Serial, CudaUVMSpace>. Previously, this is the line that had the wrong behavior, but it didn't segfault.

I'm also wondering if it's possible to make this work with execution spaces, where the execution space isn't an exact device match. For this example, dv.template modify<Exec> and dv.template sync<Exec> work for Exec = Cuda or Serial. In this PR, both of those are returning the 'host' view:

Type of dv.view<Serial>: N6Kokkos4ViewIPdJNS_10LayoutLeftENS_6DeviceINS_6SerialENS_12CudaUVMSpaceEEEEEE
Type of dv.view<Cuda>: N6Kokkos4ViewIPdJNS_10LayoutLeftENS_6DeviceINS_6SerialENS_12CudaUVMSpaceEEEEEE

[DavidPoliakoff]

@brian-kelley : oof. Back to the drawing board, I think. I think the actual logic we need is

if this thing is a Kokkos::Device
  if it matches the device
    return the device's view
if it's an exec space
  if the exec space's memory space matches
    return the memory space's view
if it's a memory space
  return that memory space's view

Thanks for letting me know, I'll integrate your example

[DavidPoliakoff]

Basically there's a lot of squishiness, where memory space and execution space both define a "device" type trait, and devices and memory spaces both define a "memory space" type trait. I'll keep at it though

[brian-kelley]

@DavidPoliakoff I agree, some of these choices will be kind of arbitrary. I don't care too much which view actually gets returned in these cases, I just want the three templated functions to agree about each type (whether it means device or host).

[DavidPoliakoff]

@brian-kelley I figured out the segfault. For some reason the view template function returns a reference, that reference is actually to a local (due to a weirdness of if_c?), so it's completely broken. Making the template view return the View by value fixes this. I'll work on the other bugs

[DavidPoliakoff]

@brian-kelley : okay, I have a candidate fix started. Note that I turned your repro into a test, and put it in TestDefaultDeviceDevelop.cpp. I'm game to talk over fixes, but you can also just push breaking tests into my test, if this doesn't do what you need. I'll polish it off for HIP and other backends later, but if we can get these tests passing with CUDA, I'll add it to our actual set of tests

[brian-kelley]

@DavidPoliakoff The test looks good and complete, there are just 2 small patches I would add:

• checks that need_sync_host() and need_sync_device() are both 0 after the "modify-device, sync-host" and "modify-host, sync-device" patterns.
• Define UVM_ENABLED_BUILD when Cuda is enabled, not just when UVM is Cuda's default memspace. Device<Cuda,CudaUVMSpace> should work with DualView the same way whether force_uvm is set or not.

I emailed you the two patches since I can't push to your fork. With those, the test builds and passes for me in Cuda+Serial+force_uvm, Cuda+OpenMP+force_uvm, and Cuda+Serial.

[DavidPoliakoff]

@brian-kelley , oh, I misunderstood what ENABLE_UVM does. I thought you couldn't use UVM without it. Good catch

[DavidPoliakoff]

Also, a Kokkos-side reviewer should think about whether this is an acceptable change. I think every change we've made here makes things more correct. But they are changes, and if somebody relied on our broken behavior, they might see differences and not be happy about them

[brian-kelley]

@brian-kelley , oh, I misunderstood what ENABLE_UVM does. I thought you couldn't use UVM without it. Good catch

We had the same confusion about that in tpetra.

[DavidPoliakoff]

Retest this please