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

non-contiguous Impl::irecv #32

Closed
wants to merge 34 commits into from
Closed

Conversation

cwpearson
Copy link
Collaborator

@cwpearson cwpearson commented Apr 12, 2024

Replacement for #9

Add an interface to KokkosComm:Req to register a callable at wait(). Use that interface to invoke unpack operations (if needed) at wait(). Add packer_type to MpiArgs.

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.

I think the Packer object and its logic should be outlined also in the source code.

src/KokkosComm_request.hpp Outdated Show resolved Hide resolved
src/KokkosComm_request.hpp Outdated Show resolved Hide resolved
src/impl/KokkosComm_irecv.hpp Outdated Show resolved Hide resolved
docs/design.rst Outdated Show resolved Hide resolved
unit_tests/test_main.cpp Outdated Show resolved Hide resolved
unit_tests/test_main.cpp Outdated Show resolved Hide resolved
unit_tests/test_isendirecv.cpp Show resolved Hide resolved
unit_tests/test_isendirecv.cpp Outdated Show resolved Hide resolved
unit_tests/test_isendirecv.cpp Outdated Show resolved Hide resolved
unit_tests/view_builder.hpp Outdated Show resolved Hide resolved
}

template <Invokable Fn>
void call_and_drop_at_wait(const Fn &f) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

For consistency with keep_until_wait(rv) consider renaming this to keep_until_wait_and_execute and the args could be the callback (unpacker) and the rv.


template <KokkosExecutionSpace ExecSpace, KokkosView RecvView, typename MpiArgs>
struct IrecvUnpacker {
IrecvUnpacker(const ExecSpace &space, RecvView &rv, MpiArgs &args)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider move into KokkosComm::Impl::Packer

using Packer = typename KCPT::packer_type;
using Args = typename Packer::args_type;

Args args = Packer::allocate_packed_for(space, "packed", rv);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe use allocate_packed_for also in the isend and send implementation

@cwpearson
Copy link
Collaborator Author

The unit test part of this is done better in #37

@dssgabriel
Copy link
Collaborator

Hi @cwpearson, what is still missing for this PR to be merged?

@cwpearson
Copy link
Collaborator Author

@dssgabriel could you please review #37 so that can go in. In the mean time, I can address some of the comments on this PR

@cwpearson cwpearson changed the title add Impl::irecv, improve unit testing output add Impl::irecv May 8, 2024
@cwpearson cwpearson force-pushed the mpi/irecv branch 2 times, most recently from 9711464 to 4d1b31d Compare May 8, 2024 15:10
@cwpearson cwpearson requested a review from devreal May 10, 2024 21:53
@cwpearson cwpearson changed the title add Impl::irecv non-contigusous Impl::irecv May 13, 2024
@cwpearson cwpearson changed the title non-contigusous Impl::irecv non-contiguous Impl::irecv May 13, 2024
@dssgabriel
Copy link
Collaborator

Apart from a review approval, is there anything else missing in this PR? It would be nice to have it merged 🙂

@masterleinad
Copy link
Collaborator

Apart from a review approval, is there anything else missing in this PR? It would be nice to have it merged 🙂

There are a lot of merge conflicts.

} else {
using SendScalar = typename SendView::value_type;
mpi_isend_fn(KCT::data_handle(sv), KCT::span(sv), mpi_type_v<SendScalar>, dest, tag, comm, &req.mpi_req());
mpi_isend_fn(KCT::data_handle(sv), KCT::span(sv), mpi_type_v<SendScalar>, dest, tag, comm, &req->mpi_req());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Somewhat out of scope, but: there needs to be a fence before the send to make sure all computation on the execution space has completed.

Comment on lines +107 to +110
template <Invokable Fn>
void call_and_drop_at_wait(const Fn &f) {
wait_callbacks_.push_back(std::make_shared<InvokableHolder<Fn>>(f));
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggest perfect forwarding to avoid copies (needs similar change to InvokableHolder):

Suggested change
template <Invokable Fn>
void call_and_drop_at_wait(const Fn &f) {
wait_callbacks_.push_back(std::make_shared<InvokableHolder<Fn>>(f));
}
template <Invokable Fn>
void call_and_drop_at_wait(Fn &&f) {
wait_callbacks_.push_back(std::make_shared<InvokableHolder<Fn>>(std::forward<Fn>(f)));
}

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.

6 participants