Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SPARSE] Add support for sparse gemv with MKLCPU #374

Merged
merged 7 commits into from
Oct 30, 2023

Conversation

Rbiessy
Copy link
Contributor

@Rbiessy Rbiessy commented Aug 31, 2023

Description

This first PR is only adding support for sparse gemv and 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 trsv and gemm are throwing unsupported for now.

Note that the examples are based on the oneapi dpcpp examples (like the other domains).

All Submissions

  • Do all unit tests pass locally? Attach a log.
  • Have you formatted the code using clang-format?

New interfaces

  • [N/A] Have you provided motivation for adding a new feature as part of RFC and
    it was accepted? # (RFC)
  • What version of oneAPI spec the interface is targeted?
    • This PR makes assumptions on the oneAPI sparse spec that are not applied yet. This matches with the close-source Sparse oneMKL spec (only some functions are not added yet).
  • Complete New features checklist

New features

  • Have you provided motivation for adding a new feature?
  • Have you added relevant tests?

@Rbiessy Rbiessy marked this pull request as draft August 31, 2023 10:29
@Rbiessy Rbiessy marked this pull request as ready for review September 7, 2023 13:34
@Rbiessy Rbiessy changed the title [Draft] [SPARSE] Enable sparse blas domain with MKLCPU backend [SPARSE] Enable sparse blas domain with MKLCPU backend Sep 7, 2023
Copy link
Contributor

@gajanan-choudhary gajanan-choudhary left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did a partial review. Great progress here, @Rbiessy. I'll continue reviewing. Some small changes might be needed for now as a first step.

examples/include/example_helper.hpp Show resolved Hide resolved
examples/include/example_helper.hpp Outdated Show resolved Hide resolved
examples/include/example_helper.hpp Show resolved Hide resolved
include/oneapi/mkl/sparse_blas/detail/sparse_blas_ct.hxx Outdated Show resolved Hide resolved
Copy link
Contributor

@gajanan-choudhary gajanan-choudhary left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did another round of review. It's looking good. I'll still need another round or two!

@Rbiessy
Copy link
Contributor Author

Rbiessy commented Sep 20, 2023

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.

@Rbiessy Rbiessy mentioned this pull request Sep 26, 2023
4 tasks
Copy link
Contributor

@mkrainiuk mkrainiuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the great work on new domain enabling! I have several questions and suggestions.

CMakeLists.txt Show resolved Hide resolved
include/oneapi/mkl/sparse_blas/sparse_blas.hpp Outdated Show resolved Hide resolved
src/sparse_blas/backends/mklcpu/CMakeLists.txt Outdated Show resolved Hide resolved
src/sparse_blas/backends/mklcpu/CMakeLists.txt Outdated Show resolved Hide resolved
tests/unit_tests/CMakeLists.txt Outdated Show resolved Hide resolved
@Rbiessy Rbiessy changed the title [SPARSE] Enable sparse blas domain with MKLCPU backend [SPARSE] Add basic structure for sparse domain using MKLCPU backend Sep 27, 2023
@Rbiessy
Copy link
Contributor Author

Rbiessy commented Oct 5, 2023

@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 gemv and optimize_gemv functions in src/sparse_blas/backends/mkl_common/mkl_operations.cxx and adding tests. You can still see the old diff here.

@Rbiessy Rbiessy changed the title [SPARSE] Add basic structure for sparse domain using MKLCPU backend [SPARSE] Add support for sparse gemv with MKLCPU Oct 5, 2023
@Rbiessy
Copy link
Contributor Author

Rbiessy commented Oct 5, 2023

I have confirmed the tests are still passing locally using 2023.2.0 oneapi package: tests_cpu.txt
Tests log using a MKL nightly that supports complex: tests_cpu_nightly.txt

Following our discussions I have removed the API for symv, trmv and gemvdot.

@@ -144,6 +144,8 @@ $> clang++ -fsycl app.o –L$ONEMKL/lib –lonemkl_blas_mklcpu –lonemkl_blas_c

Supported domains: BLAS, LAPACK, RNG, DFT

Support for SPARSE_BLAS domain is in progress and disabled by default. Use it at your own risks.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

@gajanan-choudhary gajanan-choudhary left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR is approved from my side, pending the only discussion about header naming (sparse_blas.hpp versus spblas.hpp).

Copy link
Contributor

@mkrainiuk mkrainiuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants