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

Tpetra MatrixMatrix: sort for cuSparse #13424

Merged
merged 2 commits into from
Sep 10, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion packages/tpetra/core/ext/TpetraExt_MatrixMatrix_Cuda.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,23 @@ void KernelWrappers<Scalar,LocalOrdinal,GlobalOrdinal,Tpetra::KokkosCompat::Kokk
KokkosSparse::SPGEMMAlgorithm alg_enum = KokkosSparse::StringToSPGEMMAlgorithm(myalg);

// Merge the B and Bimport matrices
const KCRS Bmerged = Tpetra::MMdetails::merge_matrices(Aview,Bview,Acol2Brow,Acol2Irow,Bcol2Ccol,Icol2Ccol,C.getColMap()->getLocalNumElements());
KCRS Bmerged = Tpetra::MMdetails::merge_matrices(Aview,Bview,Acol2Brow,Acol2Irow,Bcol2Ccol,Icol2Ccol,C.getColMap()->getLocalNumElements());


// if Kokkos Kernels is going to use the cuSparse TPL for SpGEMM, this matrix
// needs to be sorted
// Try to mirror the Kokkos Kernels internal SpGEMM TPL use logic
// inspired by https://github.com/trilinos/Trilinos/pull/11709
Comment on lines +178 to +179
Copy link
Contributor

Choose a reason for hiding this comment

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

I've followed this link and taken a look at PR #11709. However, it seems it's a closed PR, and it doesn't seem to bring the logic with the check of the cuda version.

So these are a few thoughts:

  • It may be worth double checking that is it indeed Tpetra: Permanent fix for #11655 #11709 that you intend to reference. Or perhaps there is an additional code snippet with the internal SpGEMM TPL use logic?
  • Depending on the intent of the check of the cuda version, it may be useful to see whether it could become a check of the cusparse version, with perhaps a short comment about what changed in the excluded versions.
  • It looks like in PR Tpetra: Permanent fix for #11655 #11709, the sorts were protected with checks of whether or not the matrix graph was already sorted. Would it be useful to use such checks here too?

Copy link
Contributor Author

@cwpearson cwpearson Sep 9, 2024

Choose a reason for hiding this comment

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

I think you're right about checking whether the CrsGraph is sorted, I will add that.

The point of the other logic is just to mirror what Kokkos Kernels does, since that determines whether the TPL is used. That was introduced in kokkos/kokkos-kernels#2008 and subsequently modified, I can note that in the comment as well. We should probably be checking the cusparse version rather than the CUDA version in Kokkos kernels, but that's not what we're doing today 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. Got it! :)

// and https://github.com/kokkos/kokkos-kernels/pull/2008
#if defined(KOKKOS_ENABLE_CUDA) \
&& defined(KOKKOSKERNELS_ENABLE_TPL_CUSPARSE) \
&& ((CUDA_VERSION < 11000) || (CUDA_VERSION >= 11040))
if constexpr (std::is_same_v<typename device_t::execution_space, Kokkos::Cuda>) {
if (!KokkosSparse::isCrsGraphSorted(Bmerged.graph.row_map, Bmerged.graph.entries)) {
KokkosSparse::sort_crs_matrix(Bmerged);
}
}
#endif

#ifdef HAVE_TPETRA_MMM_TIMINGS
MM = Teuchos::null; MM = rcp(new TimeMonitor (*TimeMonitor::getNewTimer(prefix_mmm + std::string("MMM Newmatrix CudaCore"))));
Expand Down
Loading