Add distributed SpGEMM - #2047
Conversation
35a5413 to
5a9c403
Compare
MarcelKoch
left a comment
There was a problem hiding this comment.
I'm still in the process of looking through it. But right now it's a bit too complicated for me to follow.
I think it can be made simpler, by just using A and B in global indices. The compression only happens at the last step, after the local C matrix has been computed (in global indices).
| * @param b the right-hand operand of the product. | ||
| * @param c a pre-created distributed Matrix (may be empty) to fill. | ||
| */ | ||
| void spgemm(ptr_param<const Matrix> b, ptr_param<Matrix> c) const; |
There was a problem hiding this comment.
I think in the Csr this is called multiply. We should keep the same name here.
| { | ||
| return partition_; | ||
| } |
There was a problem hiding this comment.
can you move this to the .cpp?
| get_row_partition() const | ||
| { | ||
| return row_partition_; | ||
| } |
| * read_distributed. May be null if the matrix was not read. | ||
| */ | ||
| std::shared_ptr<const Partition<local_index_type, global_index_type>> | ||
| get_row_partition() const |
There was a problem hiding this comment.
maybe we combine this with #2033 and store a column index map and a row index map. (The row index map would be trivial, since there are no non-local indices.)
|
|
||
| // Turns a per-rank counts vector into the corresponding exclusive-prefix-sum | ||
| // offsets vector (as used for all_to_all_v send/recv displacement arrays). | ||
| std::vector<int> counts_to_offsets(const std::vector<int>& counts) |
There was a problem hiding this comment.
why is this not using std::prefix_sum instead?
| // A's imap_ gives the remote global column indices (= B rows) this rank | ||
| // needs and their owner ranks. |
There was a problem hiding this comment.
I'm wondering if this index communication step could be handled through the collective_communicator interface.
I think if you use create_inverse and then if we would have a function create_index_map from the collective communicator, you get the rows of B to send.
There was a problem hiding this comment.
No, the index_map would need to have a create_inverse function. Although the result of that wouldn't be an index_map, since the send indices are again local indices. Maybe it should just be a create_send_indices(comm) -> array
There was a problem hiding this comment.
You can also take a look at the row_gatherer implementation. It does the same in its constructor.
| // A's imap_ gives the remote global column indices (= B rows) this rank | ||
| // needs and their owner ranks. |
There was a problem hiding this comment.
The whole communication seems overly complicated to me. I think it would be a lot simpler, if we communicated just the (device) matrix data. Then we only need to:
- create send indices
- essentially a row gather with the send indices
- communicate all rows.
| b_aug_row_ptrs[b_local_nrows + i + 1] = | ||
| static_cast<GlobalIndexType>(recv_nnz_counts[i]); | ||
| } | ||
| // Prefix-sum the remote-row counts into offsets. |
There was a problem hiding this comment.
why not use std::prefix_sum directly?
| // Remap A's global columns to B_augmented row indices via A's imap_ | ||
| // (combined index space); row_ptrs and values carry over from a_merged. |
There was a problem hiding this comment.
Is the remapping for both A and B even necessary? If we take both in global indexing, then there is only the unnecessary storage for the row_ptrs, right? The matrices can still be multiplied. I think we can live with that for now.
There was a problem hiding this comment.
We cannot use global indices and compress at the end, because rocsparse does not support spgemm in int64.
There was a problem hiding this comment.
TBH, that still seems like a better choice compared to the current implementation.
There was a problem hiding this comment.
Sorry, but I dont understand what you mean. We need to have the local spgemm's in int32 because rocsparse does not support int64. That is the reason that I had to go with the current approach.
There was a problem hiding this comment.
I mean to just not support it on AMD for now. At least until it's cleaned up more and we have found a better implementation (or support spgemm ourselves)
There was a problem hiding this comment.
I think it is important that we support AMD GPUs. I will try to simplify the code, but we should still aim to support AMD GPUs.
Also doing the local SpGEMMs in int32 is signifcantly faster, and scales better as more ranks -> more local spgemms.
There was a problem hiding this comment.
I just think that optimization is a low priority right now. We should just get it to work.
There was a problem hiding this comment.
Turns out even CUDA also only supports int64 from version > 13. So without the current approach, distributed spgemm will fail in almost all cases. Our default is Matrix<vtype, int, long>, which will not have a supported distributed spgemm.
| // The local spgemm runs with LocalIndexType (32-bit) indices, which every | ||
| // backend supports (rocSPARSE has no 64-bit spgemm). Compress B_augmented's | ||
| // global columns to a compact local space on the executor; | ||
| // b_aug_distinct_cols maps each compact index back to its global column for | ||
| // the reassemble below. |
There was a problem hiding this comment.
I think the compressing can be done using a new index map. Then we also don't need the additional compress_columns kernel.
There was a problem hiding this comment.
Also, the remapping has to match the non-local indexing for A, right?
This PR adds a simple 1D SpGEMM algorithm,$C = A \times B$ It tries to reuse the existing components from the matrix setup. The phases are:
a->spgemm(b,c)for the operationseparate_diag_off_diag.Here is the performance for a relatively dense matrix (random sparsity with ~32 nnz per row):
