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.

Sign up for GitHub

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

Jump to bottom

Enable Gemm (local) to be used with MatrixRef #969

Merged
merged 27 commits into from
Nov 24, 2023
Merged

Enable Gemm (local) to be used with MatrixRef #969

merged 27 commits into from
Nov 24, 2023

Conversation

albestro
Copy link
Collaborator

@albestro albestro commented Sep 5, 2023 •
edited
Loading

This PR partially address #915 (just local variant)

  • gemm (local) implementation working with MatrixRef (only, at the moment) with related tests for both full and sub-matrix test-cases
  • introduces scal tile extension
  • basic refactoring(=adaptation) of local_matrix and multipliable assert + introduction of same_process_grid
  • refactor test helper for sub-matrices (subValues and mixValues)
  • minor: MatrixBase string representation also print information about source_rank_index and offset

TODO

  • Check preconditions
  • Extend the test
  • Move the new function in the right place (see following note)
  • Evaluate how to deal with multipliable assertion (separate it from algorithm details)
  • Test also with ROCm (mainly if scal workaround of using gemm with nullptr works there too)

Note: GeneralSub was intended to work on submatrices, while now the sub-matrix concept is hidden by MatrixRef. For this reason, the new gemm helper might just be considered a full general multiplication, and it might be moved in multiplication::general::General (or something like that).

@albestro albestro added Type:New Feature New feature or request team:help wanted Extra attention is needed Priority:Medium labels Sep 5, 2023
@albestro albestro added this to the Optimizations milestone Sep 5, 2023
@albestro albestro self-assigned this Sep 5, 2023
This was referenced Sep 5, 2023
Merged
Closed
@msimberg
Copy link
Collaborator

msimberg commented Sep 7, 2023

This looks good to me for the current use cases. I see now what the main difficulty would be with relaxing constraints on the input matrices: ETI. I don't know what the best way around this is. Inheritance works, but if possible I'd rather not go there. We might have to try out and see how bad the compile times would be from exposing the implementations again to allow arbitrary instantiations.

@msimberg msimberg self-requested a review September 7, 2023 10:36
albestro added a commit that referenced this pull request Oct 16, 2023
@albestro
Develop: Enable Gemm (local) to be used with MatrixRef (#969)
c26c5da
@albestro albestro removed team:help wanted Extra attention is needed Priority:on hold labels Oct 27, 2023
@albestro albestro marked this pull request as ready for review October 27, 2023 13:39
rasolca
rasolca reviewed Oct 31, 2023
View reviewed changes
Copy link
Collaborator

@rasolca rasolca left a comment

Choose a reason for hiding this comment

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

Please add tests with submatrices.

test/unit/multiplication/test_multiplication_general.cpp Outdated
Comment on lines 120 to 121
GemmConfig{blas::Op::NoTrans, blas::Op::NoTrans, 21, 21, 21, 3, 3, 3},
GemmConfig{blas::Op::NoTrans, blas::Op::NoTrans, 12, 20, 11, 3, 4, 5},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add other test cases. E.g.:

  • One of the matrices has a single incomplete tile
  • a couple of other test with a full matrix

@albestro albestro requested a review from rasolca November 13, 2023 09:16
msimberg
msimberg approved these changes Nov 13, 2023
View reviewed changes
@albestro albestro force-pushed the alby/gemm-sub branch 2 times, most recently from 9f36f8b to fec1876 Compare November 16, 2023 13:37
@rasolca rasolca mentioned this pull request Nov 16, 2023
Open
@albestro
Copy link
Collaborator Author

cscs-ci run

@albestro
Copy link
Collaborator Author

cscs-ci run

@albestro
Copy link
Collaborator Author

cscs-ci run

@albestro
Copy link
Collaborator Author

Quickly tested ROCm image, which was super slow. I had a look about bindings and they seemed ok.

Anyway, I did not investigate any further about slowness, and I went on by runnning just test_multiplication_general, which passed with all green. So, the "workaround" for scal using gemm with nullptrs works correctly there too.

That's enough for what concerns this PR. 🟢

@albestro albestro requested a review from rasolca November 17, 2023 17:24
rasolca
rasolca approved these changes Nov 17, 2023
View reviewed changes
@albestro albestro mentioned this pull request Nov 20, 2023
Merged
5 tasks
@rasolca rasolca requested review from msimberg and rasolca November 21, 2023 16:10
@albestro
Copy link
Collaborator Author

cscs-ci run

@rasolca rasolca merged commit 7127a09 into master Nov 24, 2023
@rasolca rasolca deleted the alby/gemm-sub branch November 24, 2023 16:27
github-actions bot pushed a commit that referenced this pull request Nov 24, 2023
@rasolca
Doc: Enable Gemm (local) to be used with MatrixRef (#969)
58e9f40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@msimberg msimberg msimberg approved these changes

@rasolca rasolca rasolca approved these changes

@RMeli RMeli Awaiting requested review from RMeli

Assignees

@albestro albestro

Labels
Priority:Medium Type:New Feature New feature or request
Projects
DLA-F Planning
Archived in project
Milestone
Optimizations
Development

Successfully merging this pull request may close these issues.
3 participants
@albestro @msimberg @rasolca