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

Make Norm communication asynchronous #1221

Merged
merged 7 commits into from
Dec 4, 2024
Merged

Make Norm communication asynchronous #1221

merged 7 commits into from
Dec 4, 2024

Conversation

albestro
Copy link
Collaborator

Norm is one of the few points left with synchronous communication. In particular, it uses an MPI_Reduce of a single value, which is a quite special use-case since all our helpers/wrappers/functions deal with tiles instead of simple values.

Instead of making the simple value appear as a Tile, which IMHO sounds a bit cumbersome, I opted for the opposite strategy which sounds more straightforward: implement an asynchronous reduction for the single value use-case.

Main changes:

  • now the algorithm is asynchronous and it returns a sender; value is valid just on the root rank as before.
  • local reduction does not need to be synchronised anymore (now it is passed asynchronously to the distributed reduction final step)

It might be argued that:

  • we might want to make the reduction for simple value available for more algorithms: I would leave it for later in case there will be need of that;
  • we might want to keep the norm algorithm returning a value instead of a sender: as first choice I opted for changing it, but open to comments.

@albestro albestro added this to the Cleanup milestone Nov 26, 2024
@albestro albestro requested review from msimberg and rasolca November 26, 2024 16:29
@albestro albestro self-assigned this Nov 26, 2024
include/dlaf/auxiliary/norm/mc.h Outdated Show resolved Hide resolved
include/dlaf/auxiliary/norm/mc.h Outdated Show resolved Hide resolved
include/dlaf/auxiliary/norm/mc.h Outdated Show resolved Hide resolved
include/dlaf/auxiliary/norm/mc.h Outdated Show resolved Hide resolved
include/dlaf/auxiliary/norm/mc.h Outdated Show resolved Hide resolved
include/dlaf/auxiliary/norm/mc.h Outdated Show resolved Hide resolved
include/dlaf/auxiliary/norm/mc.h Outdated Show resolved Hide resolved
include/dlaf/auxiliary/norm/mc.h Show resolved Hide resolved
@albestro albestro force-pushed the alby/norm-async-comm branch 2 times, most recently from e9b3d3d to a50dcef Compare December 2, 2024 11:10
@albestro albestro marked this pull request as ready for review December 2, 2024 13:38
@albestro albestro force-pushed the alby/norm-async-comm branch from a50dcef to 62c4814 Compare December 2, 2024 14:08
Copy link
Collaborator

@msimberg msimberg left a comment

Choose a reason for hiding this comment

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

Thanks @albestro!

ETI question still open, but not a blocker for this PR (if ever).

include/dlaf/auxiliary/norm/mc.h Show resolved Hide resolved
@albestro
Copy link
Collaborator Author

albestro commented Dec 4, 2024

cscs-ci run

@rasolca rasolca merged commit 0bbec23 into master Dec 4, 2024
5 checks passed
@rasolca rasolca deleted the alby/norm-async-comm branch December 4, 2024 13:52
github-actions bot pushed a commit that referenced this pull request Dec 4, 2024
@albestro albestro mentioned this pull request Dec 9, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants