search

kokkos / kokkos-comm

Experimental MPI Wrapper for Kokkos
https://kokkos.org/kokkos-comm/
Other
12 stars 9 forks source link

Add communication modes to specialize send routines #30

Closed dssgabriel closed 3 months ago

dssgabriel commented 4 months ago

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 commented 4 months ago

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

dssgabriel commented 4 months ago

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.

dssgabriel commented 4 months ago

@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... :sweat_smile: Also, do I need to mark the lambdas as constexpr to ensure the path is correctly selected at compile-time?

dssgabriel commented 4 months ago

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 commented 4 months ago

Works for me, sorry for the delay on https://github.com/kokkos/kokkos-comm/pull/32. Open such an issue and then we will merge once conflicts are resolved!

dssgabriel commented 4 months ago

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