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 irecv wrapper in KokkosComm namespace #76

Merged
merged 6 commits into from
Jun 12, 2024
Merged

Conversation

nicoleavans
Copy link
Collaborator

Currently, irecv is only available in the Impl namespace.

dssgabriel
dssgabriel previously approved these changes Jun 6, 2024
@aprokop
Copy link
Contributor

aprokop commented Jun 11, 2024

I'd like this too for #53.

@aprokop
Copy link
Contributor

aprokop commented Jun 11, 2024

Also needs alltoall

@aprokop aprokop mentioned this pull request Jun 11, 2024
7 tasks
@@ -33,6 +34,11 @@ Req isend(const ExecSpace &space, const SendView &sv, int dest, int tag, MPI_Com
return Impl::isend<SendMode>(space, sv, dest, tag, comm);
}

template <KokkosView RecvView>
void irecv(RecvView &sv, int src, int tag, MPI_Comm comm, MPI_Request req) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why use sv for the receiving view? I think it was previously clearer with rv instead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was trying to more closely match the recv wrapper in the file:

template <KokkosExecutionSpace ExecSpace, KokkosView RecvView>
void recv(const ExecSpace &space, RecvView &sv, int src, int tag, MPI_Comm comm) {
  return Impl::recv(space, sv, src, tag, comm);
}

I agree it is clearer, I can change it back.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you can change the recv wrapper to RecvView &rv as well then 👍

@dssgabriel
Copy link
Collaborator

As the MPI_Alltoall wrapper has been implemented, consider adding it to this PR as well

template <CommMode SendMode = CommMode::Default, KokkosExecutionSpace ExecSpace, KokkosView SendView>
void send(const ExecSpace &space, const SendView &sv, int dest, int tag, MPI_Comm comm) {
return Impl::send<SendMode>(space, sv, dest, tag, comm);
}

template <KokkosExecutionSpace ExecSpace, KokkosView RecvView>
void recv(const ExecSpace &space, RecvView &sv, int src, int tag, MPI_Comm comm) {
return Impl::recv(space, sv, src, tag, comm);
void recv(const ExecSpace &space, RecvView &rv, int src, int tag, MPI_Comm comm) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@cwpearson cwpearson merged commit 2e34aef into kokkos:develop Jun 12, 2024
7 checks passed
Copy link
Contributor

@aprokop aprokop left a comment

Choose a reason for hiding this comment

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

If all we do right now is just passing things through, can't we simply do

  namespace KokkosComm {
    using Impl::recv;
    using Impl::alltoall;
  }

Edit: was too late with the review.

@nicoleavans nicoleavans deleted the pr branch June 13, 2024 14:24
@nicoleavans nicoleavans restored the pr branch June 18, 2024 16:12
@nicoleavans nicoleavans deleted the pr branch June 18, 2024 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants