Skip to content

Comments

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

Merged
Rbiessy merged 7 commits intouxlfoundation:developfrom
Rbiessy:romain/sparse
Oct 30, 2023
Merged

[SPARSE] Add support for sparse gemv with MKLCPU#374
Rbiessy merged 7 commits intouxlfoundation:developfrom
Rbiessy:romain/sparse

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.

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 Rbiessy requested a review from mkrainiuk September 11, 2023 14:26
@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.

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


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!

@Rbiessy Rbiessy merged commit d3353e1 into uxlfoundation:develop Oct 30, 2023
@Rbiessy Rbiessy deleted the romain/sparse branch October 30, 2023 09:27
normallytangent pushed a commit to normallytangent/oneMKL that referenced this pull request Aug 6, 2024
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