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

Add communication modes to specialize send routines #30

Merged
merged 35 commits into from
May 8, 2024

Conversation

dssgabriel
Copy link
Collaborator

This PR adds an enum to allow users to specify the communication mode of send routines (both blocking and non-blocking) as proposed by @cedricchevalier19 in #10. The CommMode template parameter is only used for the user-facing API: there are dedicated lean wrappers for each underlying MPI call in the Impl namespace (as suggested by @cwpearson, also in #10). Relevant unit tests have been added too.

The changes fulfill the requirements for supporting rsend and ssend from #10.

We do not support "Buffered" mode (at least for now), as it requires the user to manually attach a buffer using MPI_Buffer_attach (see documentation here).

@dssgabriel dssgabriel added the enhancement New feature or request label Apr 11, 2024
@dssgabriel dssgabriel self-assigned this Apr 11, 2024
@dssgabriel dssgabriel linked an issue Apr 11, 2024 that may be closed by this pull request
9 tasks
@dssgabriel dssgabriel removed a link to an issue Apr 11, 2024
9 tasks
@dssgabriel
Copy link
Collaborator Author

We may want to draft this for now as I haven't updated the corresponding documentation yet.

Copy link
Member

@cedricchevalier19 cedricchevalier19 left a comment

Choose a reason for hiding this comment

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

Just a quick look, but I think it makes more sense to group all the sendrecv tests in the same file.

You can define a function that take the comm "mode" as a template parameters and have tests that simplify set this parameter.

unit_tests/test_irsendrecv.cpp Outdated Show resolved Hide resolved
unit_tests/test_rsendrecv.cpp Outdated Show resolved Hide resolved
src/KokkosComm_mode.hpp Outdated Show resolved Hide resolved
src/KokkosComm_mode.hpp Outdated Show resolved Hide resolved
src/KokkosComm.hpp Outdated Show resolved Hide resolved
src/KokkosComm.hpp Outdated Show resolved Hide resolved
src/KokkosComm.hpp Outdated Show resolved Hide resolved
@dssgabriel dssgabriel marked this pull request as draft April 11, 2024 15:10
@dssgabriel dssgabriel marked this pull request as ready for review April 12, 2024 16:10
docs/api/core.rst Outdated Show resolved Hide resolved
src/impl/KokkosComm_send.hpp Outdated Show resolved Hide resolved
@dssgabriel
Copy link
Collaborator Author

The PR now does the communication mode dispatch in the Impl namespace to avoid code duplication.

It now also lets users override the default communication mode to alias CommMode::Synchronous instead of CommMode::Standard (mainly for debugging purposes) using the KOKKOSCOMM_FORCE_SYNCHRONOUS_SEND_MODE environment variable.

docs/api/core.rst Outdated Show resolved Hide resolved
docs/api/core.rst Outdated Show resolved Hide resolved
docs/api/core.rst Outdated Show resolved Hide resolved
src/impl/KokkosComm_isend.hpp Outdated Show resolved Hide resolved
docs/api/core.rst Outdated Show resolved Hide resolved
docs/api/core.rst Outdated Show resolved Hide resolved
src/KokkosComm_comm_mode.hpp Outdated Show resolved Hide resolved
Comment on lines 52 to 76
if constexpr (SendMode == CommMode::Standard) {
MPI_Isend(KCT::data_handle(args.view), args.count, args.datatype, dest,
tag, comm, &req.mpi_req());
} else if constexpr (SendMode == CommMode::Ready) {
MPI_Irsend(KCT::data_handle(args.view), args.count, args.datatype, dest,
tag, comm, &req.mpi_req());
} else if constexpr (SendMode == CommMode::Synchronous) {
MPI_Issend(KCT::data_handle(args.view), args.count, args.datatype, dest,
tag, comm, &req.mpi_req());
} else if constexpr (SendMode == CommMode::Default) {
#ifdef KOKKOSCOMM_FORCE_SYNCHRONOUS_MODE
MPI_Issend(KCT::data_handle(args.view), args.count, args.datatype, dest,
tag, comm, &req.mpi_req());
#else
MPI_Isend(KCT::data_handle(args.view), args.count, args.datatype, dest,
tag, comm, &req.mpi_req());
#endif
}
req.keep_until_wait(args.view);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd rather define a lambda so you don't have to repeat the logic for tyes that need packing or not.

src/impl/KokkosComm_send.hpp Outdated Show resolved Hide resolved
@dssgabriel
Copy link
Collaborator Author

@masterleinad Can you tell me if the lambda definitions look ok? Using auto everywhere seems to work, but I am not very proficient in C++ and not sure this is the best way to do it... 😅
Also, do I need to mark the lambdas as constexpr to ensure the path is correctly selected at compile-time?

masterleinad
masterleinad previously approved these changes Apr 19, 2024
Copy link
Collaborator

@masterleinad masterleinad left a comment

Choose a reason for hiding this comment

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

Looks OK to me.

dssgabriel and others added 11 commits April 19, 2024 17:06
MPI's `Buffered` mode requires the user to attach a buffer using `MPI_Buffer_attach`. We do not want to bother finding a way to support this cleanly for now, hence its removal.
Remove `static_assert` from unreachable path
Co-authored-by: Cédric Chevalier <cedric.chevalier019@proton.me>
Renamed the enum to `CommMode` to better reflect what it is.
Renamed the `Default` variant to `Standard` to match wording in the MPI spec.
Added comments to explain the semantics of each communication mode.
dssgabriel and others added 10 commits April 19, 2024 17:12
This change reduces code duplication in the `Impl` namespace, where the dispatch is now performed in a single interface.
We have also added an environment variable `KOKKOSCOMM_FORCE_SYNCHRONOUS_SEND_MODE` to ensure that all operations with `CommMode::Default` mode instead alias to synchronous sends instead of standard ones (mainly for debugging purposes).
If send operations do not specify a communication mode, use the default behavior.
If the `KokkosComm_FORCE_SYNCHRONOUS_MODE` is defined (through the pre-processor), the default behavior is to use `Synchronous` mode.
Otherwise, it defaults to `Standard` mode.
Co-authored-by: Daniel Arndt <arndtd@ornl.gov>
Removes reference-captures and adds explicit typing.
@dssgabriel
Copy link
Collaborator Author

I rebased the latest commits from develop onto this branch.

After discussing with @cedricchevalier19, we believe it's simpler to skip tests for ready-mode send operations for now. We should merge this PR and open an issue to fix ready-send tests while we wait for #32 to be merged.

@cwpearson
Copy link
Collaborator

Works for me, sorry for the delay on #32. Open such an issue and then we will merge once conflicts are resolved!

@dssgabriel
Copy link
Collaborator Author

Issue #43 for the skipped tests is opened. I think we can merge this now 👍

@janciesko janciesko requested a review from devreal May 6, 2024 22:44
@cwpearson cwpearson merged commit 95045cf into kokkos:develop May 8, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants