Adding more support for SpGEMM via rocsparse #1589
Conversation
|
@lucbv are you up for reviewing this? It adds some feature support for SpGEMM with rocsparse. |
|
Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
|
@seanofthemillers I'm at SC22 this week, will look at this when I get back |
There was a problem hiding this comment.
This looks fine, however it will conflict with recent changes to handle complex type. The title and description of this PR were not very explicit about including a bug fix so it was not prioritized as such...
There are also some issues related to changes to the public API which cannot be accepted. The public API needs to be deprecated before it is removed or downstream libraries (Trilinos, PETSc...) and application will break.
| void choose_default_algorithm() { | ||
| static | ||
| SPGEMMAlgorithm | ||
| default_spgemm_algorithm() { |
There was a problem hiding this comment.
This is part of Kokkos Kernels public API, you are not allowed to change it.
If you need to the new signature you can add it to the API and if for some reason you need to remove the old one, it needs to be marked as deprecated and should preferably be implemented by calling the new interface.
| { | ||
|
|
||
| /// Constructor | ||
| rocSparseSpgemmHandleType(): |
There was a problem hiding this comment.
The API of Kokkos Kernels cannot arbitrarily change, you need to deprecate the old constructor until the next major release. You also need to motivate why your new implementation is more appropriate to users than the current one.
| @@ -292,7 +310,7 @@ struct SPGEMM_JACOBI<KernelHandle, a_size_view_t_, a_lno_view_t, | |||
| Kokkos::MemoryTraits<Kokkos::Unmanaged> >, \ | |||
| false, true>; | |||
|
|
|||
| #include <KokkosSparse_spgemm_tpl_spec_decl.hpp> | |||
| // #include <KokkosSparse_spgemm_tpl_spec_decl.hpp> | |||
There was a problem hiding this comment.
This should be removed not commented.
- Adding Jacobi SpGEMM via rocsparse - Enabling rocsparse testing for SpGEMM - Adding option for non-int size types (e.g. size_t and int64_t) for SpGEMM
seanofthemillers
force-pushed
the
kk_rocsparse_spgemm_updates
branch
from
43dd4a7
to
ad6bda5
Compare
|
Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
|
@seanofthemillers @brian-kelley |
|
@lucbv Hmm... the testing and complex support are superseded now, but I haven't supported any TPLs for SpGEMM-Jacobi. I also haven't converted non-int ordinals/offsets to int to call TPLs.
|
|
@lucbv @brian-kelley Sorry, I've been a bit sidetracked. I can rewrite this to work with the new backend for spgemm-jacobi, and remove my changes for testing/complex. I'll can try to get back on this next week. I am a little worried about memory consumption for duplicating some of these arrays (index arrays and the D^{-1} * A allocation). Hiding those allocations inside a handle may not always be advisable. With the new spgemm-jacobi backend, is there a way to chose not to use rocsparse? Outside of recompiling the code with rocsparse disabled. |
|
@seanofthemillers When TPL support is done through the unification layer, the decision to call a TPL is done at compile-time and normally there isn't a way to opt out of the TPL at runtime. This design assumes that the TPL will always outperform the implementation that we write. In a few places we have alternate runtime paths for cases where TPLs perform poorly (for example, KokkosBlas::gemm where A is short-and-fat, and B is tall-and-skinny). I think in this case, the only decision to make is, should non-int index types be supported (requiring a copy of int32-typed indices in the handle)? I don't have a strong opinion either way. |
|
Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
Changes:
Adding Jacobi SpGEMM via rocsparse
Enabling rocsparse testing for SpGEMM
Enabling complex datatypes with SpGEMM
Adding option for non-int size types (e.g. size_t and int64_t) for SpGEMM