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] Update oneMKL backends to match new sparse API #500

Merged
merged 41 commits into from
Sep 6, 2024

Conversation

Rbiessy
Copy link
Contributor

@Rbiessy Rbiessy commented May 27, 2024

Description

Update sparse API to follow the specification change: uxlfoundation/oneAPI-spec#522
Also see related small changes needed in the specification: uxlfoundation/oneAPI-spec#542

This also add some backend specific documentation in the docs folder. You can see how it is rendered here: https://rbiessy.github.io/oneMKL/domains/sparse_linear_algebra.html

All Submissions

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

@Rbiessy
Copy link
Contributor Author

Rbiessy commented May 27, 2024

Test logs:
mklcpu_mklgpu_logs.txt

Tests are marked as skipped when some configurations are skipped but all the tests are run regardless.

@Rbiessy
Copy link
Contributor Author

Rbiessy commented Jun 7, 2024

Had to force push on my branch to fix my author name and email, there was no code changes since my last comment.

@Rbiessy
Copy link
Contributor Author

Rbiessy commented Jul 19, 2024

I've pushed 2f59edc to reduce the number of times get_pointer_type is called. In the other backends I had concerns this may introduce overheads for no reasons. I figured I would align the MKL backends with this.

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.

I've finally reviewed the entire PR. This is fantastic work and top notch code. Thanks a lot @Rbiessy for being patient on the review and feedback. I have some review comments, I'm ready to approve once those are resolved.

src/sparse_blas/backends/mkl_common/mkl_spmv.cxx Outdated Show resolved Hide resolved
src/sparse_blas/backends/mkl_common/mkl_spmv.cxx Outdated Show resolved Hide resolved
src/sparse_blas/backends/mkl_common/mkl_spsv.cxx Outdated Show resolved Hide resolved
tests/unit_tests/sparse_blas/include/test_common.hpp Outdated Show resolved Hide resolved
tests/unit_tests/sparse_blas/source/sparse_spmv_buffer.cpp 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.

Ready to approve after the last set of minor changes on moving around where some descriptor arguments are set and checked.

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.

Approved pending the last 2 outstanding comments about fixing exception text. Thank you for this fantastic work and for being patient while we reviewed the PR.

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.

Reaffirming LGTM given my last outstanding comments were resolved.

@Rbiessy Rbiessy merged commit c9d0b47 into oneapi-src:develop Sep 6, 2024
8 checks passed
@Rbiessy Rbiessy deleted the romain/update_sparse_mkl branch September 6, 2024 15:07
@gajanan-choudhary gajanan-choudhary added feature A request to add a new feature backend A request to enable new implementation behind API Sparse BLAS domain Sparse BLAS domain issue/request labels Sep 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend A request to enable new implementation behind API feature A request to add a new feature Sparse BLAS domain Sparse BLAS domain issue/request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants