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

Can't construct a CrsMatrix::HostMirror with its graph's HostMirror #917

Closed
brian-kelley opened this issue Mar 23, 2021 · 6 comments
Closed
Assignees

Comments

@brian-kelley
Copy link
Contributor

It's not always possible to construct a CrsMatrix::HostMirror using a CrsMatrix::StaticCrsGraphType::HostMirror.
This is a full reproducer:

#include "Kokkos_View.hpp"
#include "Kokkos_StaticCrsGraph.hpp"
#include "KokkosSparse_CrsMatrix.hpp"

int main (int narg, char **arg)
{
  Kokkos::ScopeGuard scope(narg, arg);

  using device_t = Kokkos::Device<Kokkos::DefaultExecutionSpace, Kokkos::DefaultExecutionSpace::memory_space>;

  using vals_t = Kokkos::View<double*, Kokkos::LayoutLeft, device_t>;
  using vals_mirror_t = typename vals_t::HostMirror;

  using inds_t = Kokkos::View<int*, Kokkos::LayoutLeft, device_t>;
  using inds_mirror_t = typename inds_t::HostMirror;

  using rows_nonconst_t = Kokkos::View<size_t*, Kokkos::LayoutLeft, device_t>;
  using rows_t = Kokkos::View<const size_t*, Kokkos::LayoutLeft, device_t>;
  using rows_mirror_t = typename rows_t::HostMirror::const_type;

  using graph_t = Kokkos::StaticCrsGraph<int, Kokkos::LayoutLeft, device_t, void, size_t>;
  using matrix_t = KokkosSparse::CrsMatrix<double, int, device_t, void, size_t>;

  using matrix_mirror_t = typename matrix_t::HostMirror;
  using graph_mirror_t = typename graph_t::HostMirror;

  vals_t vals("vals", 10);
  inds_t inds("inds", 10);
  rows_nonconst_t rows_nonconst("rows", 5);
  rows_t rows = rows_nonconst;

  vals_mirror_t vals_mirror = create_mirror_view_and_copy(typename vals_t::host_mirror_space(), vals);
  inds_mirror_t inds_mirror = create_mirror_view_and_copy(typename inds_t::host_mirror_space(), inds);
  rows_mirror_t rows_mirror = create_mirror_view_and_copy(typename rows_t::host_mirror_space(), rows);

  const graph_t graph(inds, rows);
  const graph_mirror_t graph_mirror(inds_mirror, rows_mirror);

  matrix_t matrix("matrix", 4, vals, graph);

  //Error on the next line: no matching constructor (graph_mirror is what fails to match)
  matrix_mirror_t matrix_mirror("matrix_mirror", 4, vals_mirror, graph_mirror);
  return 0;
}
graph_mirror type               : N6Kokkos14StaticCrsGraphIiNS_10LayoutLeftENS_9HostSpaceENS_12MemoryTraitsILj0EEEmEE
matrix_mirror ctor expects graph: N6Kokkos14StaticCrsGraphIiNS_10LayoutLeftENS_6DeviceINS_6SerialENS_9HostSpaceEEEvmEE

In this case, it's because void doesn't match MemoryTraits<0>, and Kokkos::Device<DefaultHostExecutionSpace, HostSpace> doesn't match HostSpace. In both cases, the difference doesn't really matter.

@brian-kelley brian-kelley self-assigned this Mar 23, 2021
@lucbv
Copy link
Contributor

lucbv commented Mar 23, 2021

@brian would it make more sense to create a mirror_construction that actually accepts directly a CrsMatrix as input and internally provides the proper data management between device and host instead of asking the user to do some of that himself?

using matrix_type = KokkosSparse::CrsMatrix<double, int, device_t, void, size_t>;
using matrix_mirror_type = typename matrix_type::mirror_type;

matrix_type Amat(...);
matrix_mirror_type Amat_mirror("maybe a label?", Amat);

@brian-kelley
Copy link
Contributor Author

brian-kelley commented Mar 23, 2021

@lucbv That would be possible. It would definitely need the label or some other argument to disambiguate from the existing (shallow) copy constructor.

@brian-kelley
Copy link
Contributor Author

But because of the way Tpetra separates the graph and matrix, we still need to support the original case in this reproducer

brian-kelley added a commit that referenced this issue Mar 26, 2021
Fix #917: ctor CrsMat mirror with CrsGraph mirror
@kddevin
Copy link

kddevin commented Mar 27, 2021

@brian-kelley Thank you for this fix! What is the best way to get this work into our Trilinos UVM refactor branches?

@lucbv
Copy link
Contributor

lucbv commented Mar 27, 2021

@kddevin we simply need to make a PR to Trilinos with these two files, since they are already merged in Kokkos Kernels develop the next release of Kokkos/Kokkos Kernels will not erase the changes and would be transparent with respect to these changes.
Doing the PR on Monday will also give us a couple more days of nightly testing in Kokkos Kernels which is not a bad thing : )

@kddevin
Copy link

kddevin commented Mar 27, 2021

Sounds great, @lucbv ! Thanks.

kddevin added a commit to trilinos/Trilinos that referenced this issue Mar 30, 2021
lucbv pushed a commit to lucbv/kokkos-kernels that referenced this issue May 10, 2021
Also add deep copy constructor for CrsMatrix. Takes a label and
any other CrsMatrix<...>, as long as it has same scalar/index/offset types.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants