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

Distributed matrix-matrix multiplication #16107

Closed
wants to merge 18 commits into from
Closed

Distributed matrix-matrix multiplication #16107

wants to merge 18 commits into from

Conversation

rahulghangas
Copy link
Member

@rahulghangas rahulghangas commented Jul 22, 2020 •
edited

A general distributed matrix-matrix multiplication procedure.

  • Works for multiple different distributions (Block, Cyclic, BlockCyclic)
  • Works if input matrices have different targetLocales or entirely different distributions
  • Multilocale testing - Correctness
  • Multilocale testing - Perf (is this needed if PRK DGEMM already exists)
  • Perf test against MPI reference

@rahulghangas rahulghangas marked this pull request as ready for review July 24, 2020 14:40
@rahulghangas
Copy link
Member Author

rahulghangas commented Jul 24, 2020 •
edited

We need an overload of dot or _matmatMult (preferably the second one) that takes in the output array as a parameter

@rahulghangas
Copy link
Member Author

@ben-albrecht @dgarvit @e-kayrakli @LouisJenkinsCS

@rahulghangas rahulghangas marked this pull request as draft July 24, 2020 19:45
@rahulghangas
Copy link
Member Author

Just realized there is a lot of stuff wrong with this PR. Converting back to draft

Rahul Ghangas added 3 commits July 25, 2020 06:05
Fix minor bugs and allocate local (sub)arrays only once
61fbbaf
Change to bulk communication
b722009 
Change to bulk communication since we're not creating domains for subarrays in each iteration anymore.
This solves the privatization overhead we had before
Revert back to forall based communication
b02967e
@rahulghangas rahulghangas marked this pull request as ready for review July 28, 2020 19:52
dgarvit
dgarvit approved these changes Jul 29, 2020
View reviewed changes
Copy link
Contributor

@dgarvit dgarvit left a comment

Choose a reason for hiding this comment

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

The changes look good to me!

modules/packages/LinearAlgebra.chpl Outdated Show resolved Hide resolved
dgarvit
dgarvit approved these changes Aug 1, 2020
View reviewed changes
Copy link
Contributor

@dgarvit dgarvit left a comment

Choose a reason for hiding this comment

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

LGTM! @ben-albrecht

@e-kayrakli
Copy link
Contributor

I think LinearAlgebra tests don't use distributed arrays and as noted in the OP we need those tests to be modified to use distributed arrays (maybe in combination with non-distributed ones) for correctness.

I also think that we need a performance test that uses distributed arrays with LinearAlgebra.dot even if the implementation is the same as PRK-DGEMM (it is not guaranteed to stay that way)

@rahulghangas
Copy link
Member Author

rahulghangas commented Aug 17, 2020 •
edited

Actually, I realized that it has already changed from PRK-DGEMM when I added the new block iterator and single initialization of local arrays

EDIT -- I'll update the PR with the tests

@rahulghangas
Copy link
Member Author

Will it be okay to copy the test from PRK-DGEMM as is and refactor it for distributed dot?

@rahulghangas
Copy link
Member Author

@e-kayrakli I've updated the PR with tests (both performance and correctness). Can you do a sanity check?

@e-kayrakli
Copy link
Contributor

Looked good on a quick look, will test and merge on Monday.

@rahulghangas
Copy link
Member Author

Fixed. I'm running the test now to make sure it compiles and runs

@rahulghangas
Copy link
Member Author

Compiles and runs

@e-kayrakli e-kayrakli self-assigned this Nov 17, 2020
@e-kayrakli
Copy link
Contributor

I'll re-test and review this this week. @rahulghangas -- sorry for the delay with the release and everything. If you could summarize what this PR is about in the PR message, that'd be great. Remember that they go into git history directly, so details can be helpful. Thanks!

@e-kayrakli
Copy link
Contributor

Based on silence (and conflicting files), I am closing this PR.

@rahulghangas -- Please feel free to reopen when you have the time to work on this.

@e-kayrakli e-kayrakli closed this Dec 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dgarvit dgarvit dgarvit approved these changes

@e-kayrakli e-kayrakli Awaiting requested review from e-kayrakli

Assignees

@e-kayrakli e-kayrakli

Labels
post-release stat: cla signed stat: developed under CLA
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@rahulghangas @e-kayrakli @dgarvit @ben-albrecht @bradcray @chapelautomaton