[SPARSE] Add support for sparse gemv with MKLCPU#374
[SPARSE] Add support for sparse gemv with MKLCPU#374Rbiessy merged 7 commits intouxlfoundation:developfrom Rbiessy:romain/sparse
Conversation
gajanan-choudhary
left a comment
There was a problem hiding this comment.
Did a partial review. Great progress here, @Rbiessy. I'll continue reviewing. Some small changes might be needed for now as a first step.
include/oneapi/mkl/sparse_blas/detail/onemkl_sparse_blas_backends.hxx
Outdated
Show resolved
Hide resolved
include/oneapi/mkl/sparse_blas/detail/onemkl_sparse_blas_backends.hxx
Outdated
Show resolved
Hide resolved
gajanan-choudhary
left a comment
There was a problem hiding this comment.
Did another round of review. It's looking good. I'll still need another round or two!
|
Made some more changes to follow some spec updates and make it easier to add MKLGPU backend in the future. Please let me know if you have more feedback. |
mkrainiuk
left a comment
There was a problem hiding this comment.
Thanks for the great work on new domain enabling! I have several questions and suggestions.
include/oneapi/mkl/sparse_blas/detail/onemkl_sparse_blas_backends.hxx
Outdated
Show resolved
Hide resolved
|
@gajanan-choudhary I am sorry I squashed all the commits and forgot to keep the gemv commit separate. In a nutshell the gemv commit was only calling the MKLCPU |
|
I have confirmed the tests are still passing locally using 2023.2.0 oneapi package: tests_cpu.txt Following our discussions I have removed the API for |
|
|
||
| Supported domains: BLAS, LAPACK, RNG, DFT | ||
|
|
||
| Support for SPARSE_BLAS domain is in progress and disabled by default. Use it at your own risks. |
There was a problem hiding this comment.
@mmeterel I think I understand better your concern about adding API that throw unsupported now. If the risk is that users may run into unexpected errors, is adding this line in the README helping?
From my perspective it was more efficient to add all the APIs in one go and the risk of people using it is low given that sparse_blas is disabled by default.
examples/sparse_blas/compile_time_dispatching/sparse_blas_gemv_usm_mklcpu.cpp
Show resolved
Hide resolved
examples/sparse_blas/compile_time_dispatching/sparse_blas_gemv_usm_mklcpu.cpp
Outdated
Show resolved
Hide resolved
gajanan-choudhary
left a comment
There was a problem hiding this comment.
PR is approved from my side, pending the only discussion about header naming (sparse_blas.hpp versus spblas.hpp).
mkrainiuk
left a comment
There was a problem hiding this comment.
Looks good to me, thank you!
Description
This first PR is only adding support for sparse
gemvand the necessary functions to set it up. This is only adding the MKLCPU backend for now. Compliant with the provisional oneMKL specification v1.3.The operations
trsvandgemmare throwing unsupported for now.Note that the examples are based on the oneapi dpcpp examples (like the other domains).
All Submissions
New interfaces
it was accepted? # (RFC)
New features