Conversation
cedricchevalier19
left a comment
cedricchevalier19
left a comment
There was a problem hiding this comment.
I think the Packer object and its logic should be outlined also in the source code.
src/KokkosComm_request.hpp
Outdated
| } | ||
|
|
||
| template <Invokable Fn> | ||
| void call_and_drop_at_wait(const Fn &f) { |
There was a problem hiding this comment.
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.
src/impl/KokkosComm_irecv.hpp
Outdated
|
|
||
| template <KokkosExecutionSpace ExecSpace, KokkosView RecvView, typename MpiArgs> | ||
| struct IrecvUnpacker { | ||
| IrecvUnpacker(const ExecSpace &space, RecvView &rv, MpiArgs &args) |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
Maybe use allocate_packed_for also in the isend and send implementation
|
The unit test part of this is done better in #37 |
|
Hi @cwpearson, what is still missing for this PR to be merged? |
|
@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
changed the title
Impl::irecv, improve unit testing outputImpl::irecv
cwpearson
force-pushed
the
mpi/irecv
branch
2 times, most recently
from
9711464 to
4d1b31d
Compare
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. Make failures off rank 0 somewhat visible in the unit tests.
Co-authored-by: Daniel Arndt <arndtd@ornl.gov>
Co-authored-by: Daniel Arndt <arndtd@ornl.gov>
Co-authored-by: Daniel Arndt <arndtd@ornl.gov>
|
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()); |
There was a problem hiding this comment.
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.
| template <Invokable Fn> | ||
| void call_and_drop_at_wait(const Fn &f) { | ||
| wait_callbacks_.push_back(std::make_shared<InvokableHolder<Fn>>(f)); | ||
| } |
There was a problem hiding this comment.
Suggest perfect forwarding to avoid copies (needs similar change to InvokableHolder):
| 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))); | |
| } |
6 participants
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.