Add rocSPARSE CSR SpGEMM by yhmtsai · Pull Request #51 · SparseBLAS/spblas-reference

yhmtsai · 2025-03-27T18:24:48Z

Summary:
This PR adds rocSPARSE CSR SpGEMM with 3 args (C = alpha A * B), 4 args (C = alpha A * B + beta D), and the symbolic/numeric phase.

Details:

Add spgemm state
Add 4 arguments version of SpGEMM (C = alpha A * B + beta D)
the 3 arguments is passing a null matrix D into 4 arguments version
also adds the symbolic_compute, symbolic_fill, and numeric stage
currently, three type of test: spgemm similar to the current CPU version, spgemm_reuse: checking symbolic/numeric, and spgemm_4args: checking 4 arguments version

potential TODO:

create a structure to handle the rocsparse handle in one place
helper for create rocsparse_mat (is already merged in another pr)

Merge Checklist:

Passing CI
Update documentation or README.md
Additional Test/example added (if applicable) and passing
At least one reviewer approval
(optional) Clang sanitizer scan run and triaged
Clang formatter applied (verified as part of passing CI)

YvanMokwinski

I have a general comment: I might not be used to cutting-edge C++, but I don't find the code friendly to read.

include/spblas/vendor/rocsparse/multiply_spgemm.hpp

YvanMokwinski · 2025-04-02T17:04:58Z

include/spblas/vendor/rocsparse/multiply_spgemm.hpp

+        to_rocsparse_indextype<typename output_type::offset_type>(),
+        to_rocsparse_indextype<typename output_type::index_type>(),
+        rocsparse_index_base_zero,
+        to_rocsparse_datatype<typename output_type::scalar_type>()));


maybe we could have something like:

using output_scalar_t = typename output_type::scalar_type;
It would simplify the writings.

I have merged them into a helper function to create a matrix handle easily from the csr_view

YvanMokwinski · 2025-04-02T17:15:10Z

include/spblas/vendor/rocsparse/multiply_spgemm.hpp

+        to_rocsparse_datatype<value_type>(), rocsparse_spgemm_alg_default,
+        rocsparse_spgemm_stage_buffer_size, &buffer_size, nullptr));
+    if (buffer_size > this->buffer_size_) {
+      this->alloc_.deallocate(workspace_, this->buffer_size_);


let's have a reallocate method in the allocator?

I think it is close to the array more than the allocator?
do you imagine something like alloc_.reallocate(&workspace_, old_buffer_size_, new_buffer_size_)?

include/spblas/vendor/rocsparse/multiply_spgemm.hpp

YvanMokwinski · 2025-04-02T17:22:15Z

include/spblas/vendor/rocsparse/multiply_spgemm.hpp

+    tensor_scalar_t<A> alpha = alpha_optional.value_or(1);
+    value_type alpha_val = alpha;
+    auto beta_optional = __detail::get_scaling_factor(d);
+    value_type beta = beta_optional.value_or(1);


I am not sure that relying on a and d types for alpha and beta is the best approach.

include/spblas/vendor/rocsparse/multiply_spgemm.hpp

YvanMokwinski · 2025-04-02T17:25:48Z

include/spblas/vendor/rocsparse/multiply_spgemm.hpp

+           __detail::is_csr_view_v<C>
+void multiply_compute(spgemm_state_t& spgemm_handle, A&& a, B&& b, C&& c) {
+  auto d = __rocsparse::create_null_matrix<std::remove_reference_t<C>>();
+  spgemm_handle.multiply_compute(a, b, c, scaled(0.0, d));


why do we have either scaled(0.0, d) or d?

It is to reuse the 4 arguments SpGEMM. It will give the same setup when using rocsparse_spgemm for C = A*B, null_matrix set the all pointer to nullptr.

I want to note that if down the road, we do start to enforce the difference in behaviour for

C = alpha op(A)*op(B)
C = alpha op(A)op(B) + betaop(D)

with regards to beta, with the first being a strong ZERO for beta meaning we write into C and values didn't need to be initialized, vs the second one with beta == 0.0, still having possibility of propogating NaNs or Infs and not going like BLAS does of saying beta == 0.0 leads to not accessing D. Right now this test as written does not make this distinction and the 3 arg test would fail if D had nans in it...

BenBrock · 2025-04-17T20:21:16Z

@yhmtsai What's the status of this PR? Anything required on my part here, or are you still updating, waiting for @YvanMokwinski to comment on your most recent updates?

yhmtsai · 2025-06-05T13:43:25Z

@YvanMokwinski @BenBrock forgot to mention it can be checked again.
I decide not to introduce a class to handle the rocsparse handle in this PR.
I am also thinking whether using shared_ptr to handle rocsparse matrix handle but I keep it as current state until matrix_opt.
I think 4arg SpGEMM are not supported by the other backend, so I keep them in rocsparse only

BenBrock

Hi @yhmtsai, looks mostly good, I just have a couple of small comments.

As a favor to me, can you please avoid performing Git force pushes on branches for which you have an open PR? If no one is looking at your code yet, force pushing is mostly fine. However, if you force push to a branch I've already reviewed, it becomes very difficult to see what's changed recently. Please use merge commits instead of rebasing if necessary.

BenBrock · 2025-06-09T20:58:59Z

include/spblas/vendor/rocsparse/descriptor.hpp

+  throw_if_error(rocsparse_create_csr_descr(
+      &descr, __backend::shape(a)[0], __backend::shape(a)[1], a.values().size(),
+      a.rowptr().data(), a.colind().data(), a.values().data(),
+      to_rocsparse_indextype<typename matrix_type::offset_type>(),


Please use the tensor_traits inspector instead of depending on scalar_type, index_type, and offset_type typedefs. This is cleaner (you can directly write tensor_scalar_t<mat>) and will work with types we don't define like mdspan as well.

will it be uniform for the other types? like Coo, offset_type does not make sense.

BenBrock · 2025-06-09T21:00:28Z

include/spblas/vendor/rocsparse/multiply_spgemm.hpp

+
+} // namespace __rocsparse
+
+class spgemm_state_t {


This is okay for now, but eventually we need to have one operation_state_t that encapsulates this (if that's how you want to design it).

Yes, if the operation_state_t is for user-friendly in the end. for now, I think separating individually should be clearer and easier for future change

BenBrock · 2025-06-09T21:02:28Z

include/spblas/vendor/rocsparse/multiply_spgemm.hpp

+template <matrix A, matrix B, matrix C>
+  requires __detail::has_csr_base<A> && __detail::has_csr_base<B> &&
+           __detail::is_csr_view_v<C>
+void multiply_inspect(spgemm_state_t& spgemm_handle, A&& a, B&& b, C&& c) {}


We need to at least add a version that takes in three matrices and returns a state (see multiply.hpp).

yhmtsai · 2025-06-26T14:18:34Z

@BenBrock Sure, I can avoid force rebasing. It is what we need to do to keep linear history in Ginkgo and I try to reorganize the commit history to describe the change. As we always do squash, my commit history will be merged together anyway.

…_spgemm

yhmtsai · 2025-08-07T14:26:50Z

Hi @BenBrock I have used merge way to update this branch. hope this will help the review process.
I only do adaption on changes in this pr not revert something in this pr here

BenBrock

Have one comment about the CMakeLists.txt, but otherwise looks fine to merge.

BenBrock · 2025-08-20T00:18:56Z

test/gtest/CMakeLists.txt

-
+function(add_rocsparse_lang file_list)
+  if (ENABLE_ROCSPARSE)
+    set_source_files_properties(${${file_list}} PROPERTIES LANGUAGE HIP)


Could we perhaps just have this inline? Personally I find that easier to read than needing to go to a separate function.

yes. agree with it. It is quite small so no need for function now

Co-authored-by: Benjamin Brock <brock@cs.berkeley.edu>

yhmtsai · 2025-08-21T11:06:07Z

after push, it automatically dismiss the review. @BenBrock could you check it again?

…_spgemm

srobertp

these changes look good to me as well. Approved for merge

…_spgemm

spencerpatty

approved with my other github id, looks good!

yhmtsai self-assigned this Mar 27, 2025

YvanMokwinski reviewed Apr 2, 2025

View reviewed changes

yhmtsai force-pushed the dev/yhmtsai/rocsparse_spgemm branch from 605f085 to 64d662f Compare April 17, 2025 14:43

yhmtsai added 7 commits May 22, 2025 15:35

add rocsparse spgemm

405bb91

add rocsparse spgemm with 3 args test

9263ec1

add rocsparse spgemm with 4 args test

59cfabe

add rocsparse spgemm symbolic/numeric test

ed7d26f

add the test when changing the pointer

e7ecada

create a matrix descriptor helper

4595c8d

add vector descriptor helper

eeb3a67

yhmtsai force-pushed the dev/yhmtsai/rocsparse_spgemm branch from c39ef55 to eeb3a67 Compare May 22, 2025 14:10

review update

59c0b7d

yhmtsai marked this pull request as ready for review May 22, 2025 14:49

BenBrock previously approved these changes Jun 9, 2025

View reviewed changes

yhmtsai added 2 commits August 7, 2025 16:13

Merge remote-tracking branch 'origin/main' into dev/yhmtsai/rocsparse…

28379cd

…_spgemm

fix and adapt the changes from main

5f9c090

yhmtsai dismissed BenBrock’s stale review via 5f9c090 August 7, 2025 14:16

BenBrock previously approved these changes Aug 20, 2025

View reviewed changes

fix meaningless cmake function

e36a516

Co-authored-by: Benjamin Brock <brock@cs.berkeley.edu>

yhmtsai dismissed BenBrock’s stale review via e36a516 August 20, 2025 14:08

Merge remote-tracking branch 'origin/main' into dev/yhmtsai/rocsparse…

3b58452

…_spgemm

srobertp approved these changes Mar 25, 2026

View reviewed changes

Merge remote-tracking branch 'origin/main' into dev/yhmtsai/rocsparse…

28fc8be

…_spgemm

spencerpatty approved these changes Mar 26, 2026

View reviewed changes

yhmtsai merged commit a138942 into main Mar 26, 2026
9 of 12 checks passed

yhmtsai deleted the dev/yhmtsai/rocsparse_spgemm branch March 26, 2026 15:38

Conversation

yhmtsai commented Mar 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

YvanMokwinski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BenBrock commented Apr 17, 2025

Uh oh!

yhmtsai commented Jun 5, 2025

Uh oh!

BenBrock left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yhmtsai commented Jun 26, 2025

Uh oh!

yhmtsai commented Aug 7, 2025

Uh oh!

BenBrock left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yhmtsai commented Aug 21, 2025

Uh oh!

srobertp left a comment

Choose a reason for hiding this comment

Uh oh!

spencerpatty left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

yhmtsai commented Mar 27, 2025 •

edited

Loading

BenBrock left a comment •

edited

Loading