Add matrix_opt into oneMKL_SYCL backend #39

spencerpatty · 2025-02-28T15:46:14Z

I am concerned about the operation_state_t complexity with regards to SpGEMM C matrix_handle_t is stored in operation_state_t that keeps the continuity between compute/fill. There is a matrix_opt restriction on C right now, but it seems it would be important to let C be a matrix_opt, that way it isn't stored in the operation_state_t indefinitely... lifetime is complicated

spencerpatty · 2025-03-17T22:27:49Z

@BenBrock any thoughts about integrating the matrix_opt into our solutions now that we have CSC and transpose objects merged to main branch ?

I seem to recall there were some open questions we had discussed a while ago:

should matrix_opt be a base_view ? I think we decided it should not, but there should be another request which gives the usable_view (name to be refined) which is either a base xyz_view or a matrix_opt containing an xyz_view object.
should the matrix_opt be created with an execution_policy, so that when appropriate, a queue/stream can also be available for destruction, or should we add a member function to matrix_opt like clean(ExecutionPolicy & policy) that enqueues (if appropriate) the deletion of any internals stored...
Separate topic, but might fit here while modifying the base stuff like we will need to do about handling "const" data types more appropriately, with our tensor_index_t<A> etc functions, to have some with const and others with const removal for internals before we get too far along with backend impls :) See 68a4e3b for test case where @YvanMokwinski and I were able to succeed at SpMV with partially const csr_view data (and onemkl needed to const_cast since we don't have a const matrix operator construct...)

BenBrock

Hi @spencerpatty—I've merged this PR with main to get it up-to-date and then made a couple of small tweaks. (Actually some of the changes are to pre-existing code to make it more consistent.)

The main change is switching to get_matrix_handle, which will handle (no pun intended) the cases where you do and don't have a matrix_opt.

I think the design as-is is pretty reasonable.

The one thing I might consider changing is how we store the matrix handles inside of operation_state. Currently, we store a nullptr handle if there is a matrix_opt and a non-nullptr handle if there isn't. We could update this to store a wrapper object inside the operation_state. The wrapper object could then keep track of whether it ought to delete itself or not, which would simplify things considerably.

Still, I think this is fine as-is. I'm approving this and will send you a review request right back to review my changes/the current state.

BenBrock · 2025-04-17T22:29:11Z

include/spblas/vendor/onemkl_sycl/spgemm_impl.hpp

+  O* c_rowptr = (O*) info.state_.c_rowptr;

-  I* c_rowptr = (I*) info.state_.c_rowptr;
+  auto a_handle = __mkl::get_matrix_handle(q, a, info.state_.a_handle);


This is the main update I made—instead of calling get_ultimate_base, I re-worked get_matrix_handle to work directly on a stack of views. It will iterate through and grab the matrix_opt if available, or else create a new view.

aha, I see. And it is an MKL specific api (instead of a general one) since we are returning an MKL specific object... this is good. Others can add their handlers as well for their objects.

so in here, I guess we don't ever interact with the ultimate_base of a or b, but rather we always get an mkl matrix handle from a or info.state including extracting the xyz_view and creating a matrix_handle from it, in thus get_matrix_handle ...

BenBrock · 2025-04-17T22:29:31Z

include/spblas/vendor/onemkl_sycl/spgemm_impl.hpp

      index<>{__backend::shape(c)[0], __backend::shape(c)[1]}, nnz,
-      __mkl::operation_state_t{a_handle, b_handle, c_handle, nullptr, descr,
-                               (void*) c_rowptr, q}};
+      __mkl::operation_state_t{__detail::has_matrix_opt(a) ? nullptr : a_handle,


The function has_matrix_opt will return whether or not a stack of views has a matrix_opt, which you can then use to handle those two cases.

BenBrock · 2025-04-17T22:34:52Z

@spencerpatty GitHub won't allow me to request a review, so please take a look and let me know what you think.

spencerpatty · 2025-04-18T14:32:52Z

include/spblas/detail/view_inspectors.hpp

+  if constexpr (has_base<T>) {
+    if constexpr (is_matrix_opt_view_v<T>) {
+      return t;
    } else {
-        return t;
+      return get_ultimate_base(t.base());
    }
+  } else {
+    return t;
+  }


so get_ultimate_base still returns not the ultimate base but either a matrix_opt or the bottom xyz_view now ? do we want to keep that as is ? or should we return it back to ignoring matrix_opt_view_v ?

spencerpatty · 2025-04-18T14:33:25Z

include/spblas/backend/view_customizations.hpp


 template <typename M>
-  requires(__detail::is_matrix_instantiation_of_mdspan_v<M>)
+  requires(__detail::is_matrix_mdspan_v<M>)


nice, I like this shortened names

spencerpatty · 2025-04-18T14:34:40Z

include/spblas/concepts.hpp

+concept matrix = __detail::is_csr_view_v<M> || __detail::is_csc_view_v<M> ||
+                 __detail::is_matrix_mdspan_v<M> || __detail::matrix<M>;


I am guessing the "is_xyz_view_v" look through the matrix_opt to see the xyz_view underneath so a matrix_opt would also be classified properly here ...

spencerpatty · 2025-04-18T14:35:25Z

include/spblas/detail/view_inspectors.hpp

+template <tensor T>
+bool has_matrix_opt(T&& t) {
+  if constexpr (is_matrix_opt_v<T>) {
+    return true;
+  } else if constexpr (has_base<T>) {
+    return has_matrix_opt(t.base());
+  } else {
+    return false;
+  }
+}
+


nice checker

aha, get_ultimate_base was returned to it's previous state and we use has_matrix_opt() and the custom __mkl::get_matrix_handle() to get matrix handle now, instead of having ultimate_base return it . I like this much more. Good changes!

spencerpatty · 2025-04-18T14:45:14Z

include/spblas/vendor/onemkl_sycl/detail/get_matrix_handle.hpp


    return m.matrix_handle_;
+  } else if constexpr (__detail::has_base<M>) {
+    return get_matrix_handle(q, m.base(), handle);


oh, so line 29 and 30 will iterate through the stack and keep looking until we either find a matrix_handle, or we run out of bases and check either handle or create_matrix_handle on ultimate base of m ...

spencerpatty · 2025-04-18T14:50:09Z

include/spblas/views/matrix_opt_impl.hpp

+  auto base() {
+    return matrix_;
+  }
+
+  auto base() const {
+    return matrix_;
+  }


why do we need both const (accessor) and nonconst(mutator) functions for base() ? can we really guarantee that the accessor won't modify matrix_ ?

spencerpatty · 2025-04-18T15:32:30Z

include/spblas/vendor/onemkl_sycl/spmm_impl.hpp

+  if (!__detail::has_matrix_opt(a)) {
+    oneapi::mkl::sparse::release_matrix_handle(q, &a_handle).wait();
+  }


aha, this is good to enable a_handle to persist in matrix_opt ... good

spencerpatty · 2025-04-18T15:34:08Z

include/spblas/vendor/onemkl_sycl/spmv_impl.hpp

+  if (!__detail::has_matrix_opt(a)) {
+    oneapi::mkl::sparse::release_matrix_handle(q, &a_handle).wait();
+  }


so we turned on matrix_opt for spmv/spmm and spgemm ... but it looks like there is still some work to add it to triangular_solve. @BenBrock do you have opinion about whether we do that in a separate PR or in this one ?

spencerpatty

As this was originally my PR, I am not able to approve it, but I have reviewed the changes that @BenBrock made and approve of the PR now for merge. (There may be some thigns listed below that we want to add to this PR, or it is stable right now, so is fine to merge)

Summary: We add in the matrix_opt handling to spmv/spmm/spgemm, though it is not in TRSV yet. We added a new has_matrix_opt() function and a custom _mkl::get_matrix_handle() in these functions to isolate the interaction between matrix M and getting a vendor matrix object for calling the vendor APIs.

TODOs:

add matrix_opt support to triangular_solve
Add use of matrix_opt into examples or testing

BenBrock · 2025-04-24T18:49:49Z

@spencerpatty Good point about testing and triangular_solve. I think we can wait for another PR to support triangular_solve.

I want to implement some more comprehensive testing that will test with every possible combination of arguments (raw view, scaled, matrix_opt, scaled matrix_opt, etc.), but I think that will have to wait. We already have one SpMM test that uses matrix_opt, so I'll add an SpGEMM one and merge for now.

BenBrock

Approving to merge based on @spencerpatty's comments.

Add matrix_opt into oneMKL_SYCL backend

9b95b2d

spencerpatty self-assigned this Feb 28, 2025

spencerpatty requested a review from BenBrock February 28, 2025 15:46

BenBrock mentioned this pull request Mar 3, 2025

Implement csc_view, implement transposed. #30

Merged

BenBrock and others added 4 commits April 17, 2025 13:57

Merge branch 'main' into dev/spatty/integrate_matrix_opt_to_mkl_sycl

a82978d

Fix formatting

b80d83b

Update to support mixed CSR/CSC.

9ac5b90

Update to avoid breaking other vendor backends.

079c37f

BenBrock previously approved these changes Apr 17, 2025

View reviewed changes

BenBrock marked this pull request as ready for review April 17, 2025 22:34

spencerpatty commented Apr 18, 2025

View reviewed changes

BenBrock added 2 commits April 24, 2025 11:38

Merge branch 'main' into dev/spatty/integrate_matrix_opt_to_mkl_sycl

b3298e1

Fix formatting

cbc2cd7

BenBrock dismissed their stale review via cbc2cd7 April 24, 2025 18:40

Add SpGEMM test using matrix_opt.

034f503

BenBrock self-requested a review April 24, 2025 18:53

BenBrock approved these changes Apr 24, 2025

View reviewed changes

BenBrock merged commit 7f9de98 into main Apr 24, 2025
20 of 22 checks passed

BenBrock deleted the dev/spatty/integrate_matrix_opt_to_mkl_sycl branch April 24, 2025 18:53

		concept matrix = __detail::is_csr_view_v<M> \|\| __detail::is_csc_view_v<M> \|\|
		__detail::is_matrix_mdspan_v<M> \|\| __detail::matrix<M>;

Add matrix_opt into oneMKL_SYCL backend #39

Add matrix_opt into oneMKL_SYCL backend #39

Uh oh!

Conversation

spencerpatty commented Feb 28, 2025

Uh oh!

spencerpatty commented Mar 17, 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!

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!

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!

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!

spencerpatty left a comment

Choose a reason for hiding this comment

Uh oh!

BenBrock commented Apr 24, 2025

Uh oh!

BenBrock left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants