Skip to content

Comments

[SPARSE] Add support for sparse GEMM using MKLCPU#403

Merged
Rbiessy merged 10 commits intouxlfoundation:developfrom
Rbiessy:romain/sparse_gemm
Nov 15, 2023
Merged

[SPARSE] Add support for sparse GEMM using MKLCPU#403
Rbiessy merged 10 commits intouxlfoundation:developfrom
Rbiessy:romain/sparse_gemm

Conversation

@Rbiessy
Copy link
Contributor

@Rbiessy Rbiessy commented Oct 23, 2023

Description

Add support for sparse GEMM using MKLCPU.
This relies on #402 and #374

Checklist

All Submissions

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

New features

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

@Rbiessy
Copy link
Contributor Author

Rbiessy commented Oct 23, 2023

Log using a recent close-source MKL and DPCPP builds, all tests are passing: sparse_gemm_log.txt
I confirmed that with the 2023.2.0 release everything is building and the tests are either skipped or are passing.

@Rbiessy
Copy link
Contributor Author

Rbiessy commented Oct 30, 2023

I have rebased the branch and confirmed that this is still building and running fine with oneapi 2023.2.0 as well as with recent DPCPP + MKL builds (see log above).

@Rbiessy Rbiessy changed the title Add support for sparse GEMM using MKLCPU [SPARSE] Add support for sparse GEMM using MKLCPU Oct 30, 2023
@noffermans
Copy link
Contributor

This PR looks good to me! I mostly had questions for my own sake.
I'll wait to see whether @gajanan-choudhary wants to review the PR himself before giving approval.

Rbiessy and others added 2 commits October 31, 2023 13:10
Co-authored-by: noffermans <126679373+noffermans@users.noreply.github.com>
Co-authored-by: noffermans <126679373+noffermans@users.noreply.github.com>
Copy link
Contributor

@FMarno FMarno left a comment

Choose a reason for hiding this comment

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

I think it looks good overall. Feel free to ignore my comment.

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.

Once you add the second optimize_gemm() API, I will approve. Overall the changes look fine.
I just need to take a closer look at the gemm unit test to understand ldb/ldc/inner dim/outer dim, etc. and the combinations that play out in there, but that is not a blocker for 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.

LGTM!

@Rbiessy Rbiessy merged commit dd082b6 into uxlfoundation:develop Nov 15, 2023
@Rbiessy Rbiessy deleted the romain/sparse_gemm branch November 15, 2023 09:24
normallytangent pushed a commit to normallytangent/oneMKL that referenced this pull request Aug 6, 2024
Co-authored-by: noffermans <126679373+noffermans@users.noreply.github.com>
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.

4 participants