coll/libnbc: correctly handle MPI_BOTTOM #9651
Conversation
ggouaillardet
force-pushed
the
topic/nbc_mpi_bottom
branch
from
63dc01c to
a92266c
Compare
|
java failure bot:aws:retest |
| { \ | ||
| inplace = 0; \ | ||
| if(recvbuf == sendbuf) { \ | ||
| if(recvbuf == sendbuf && MPI_BOTTOM != sendbuf) { \ |
There was a problem hiding this comment.
Is this test really valid ? The MPI standard prohibits using a recvbuf equal to the sendbuf to indicate an IN_PLACE operation. Why is this macro accepting it ?
There was a problem hiding this comment.
I get your point, but are you 100% sure about this?
Bad things will definitely occurs if sendbuf == recvbuf and for example the same datatype is used.
But if the user pass sendbuf == recvbuf with for example two datatypes that describes non overlapping subarrays, would that violate the standard? if yes, is there an explicit exception for MPI_BOTTOM ?
Also, should we enhance the C bindings and error out with a user friendly error code if the user passes sendbuf == recvbuf (unless MPI_BOTTOM) ?
There was a problem hiding this comment.
Correct, the standard prohibit overlapping regions between the typemap of the send and receive buffers. So, while it is legal to use sendbuf == recvbuf it can never represent an IN_PLACE operation. So, that macro seems a little extreme in the detection of IN_PLACE.
|
@ggouaillardet What's the status of this PR? Are you going to merge it as is despite the concerns about the macro? |
|
I'll be supportive to merge despite the issue with the macro for 2 reasons. First, it is not a regression, and second I think most collective already have protection against this case in the upper layer MPI_ API. |
|
@ggouaillardet Could you rebase your branch to latest |
if both send and receive buffers are MPI_BOTTOM, this is not equivalent to MPI_IN_PLACE Thanks Lisandro Dalcin for reporting this issue. Refs. open-mpi#9650 Signed-off-by: Gilles Gouaillardet <[email protected]>
ggouaillardet
force-pushed
the
topic/nbc_mpi_bottom
branch
from
a92266c to
88aad4d
Compare
|
@dalcinl I just rebased my branch on top of ompi/master |
|
@ggouaillardet Your branch passed all mpi4py tests. From my side, this PR is ready for merging. |
|
@bosilca let's merge this PR for now and think about a better fix. |
|
Will you merge this fix to branches |
|
v5.0.x: #9723 |
4 participants
if both send and receive buffers are MPI_BOTTOM, this is not
equivalent to MPI_IN_PLACE
Thanks Lisandro Dalcin for reporting this issue.
Refs. #9650
Signed-off-by: Gilles Gouaillardet [email protected]