comm/cid: use ibcast to distribute result in intercomm case

[hjelmn]

This commit updates the intercomm allgather to do a local comm bcast
as the final step. This should resolve a hang seen in intercomm
tests.

Signed-off-by: Nathan Hjelm hjelmn@me.com

[hjelmn]

@ggouaillardet This should resolve the hang. Can you verify?

[ggouaillardet]

Thsnks, will try.

Btw, why do we iallreduce ?
Could we ireduce with root=0 instead ?

[ggouaillardet]

I think i understand better now ...
ibcast on the intercomm basically cannot work since it is not bidirectional, hence iallgatherv, but with scount=1 on roots and scount=0 elsewhere, and rcounts[0]=1 and rcounts[i>0]=0 everywhere
These settings had gone during the refactor process.
Anyway, that was an overkill since ibcast on the local comm is just what is needed

I am confident ireduce can be used instead of iallreduce, and so we can only allocate tmpbuf on roots

Also, allreduce_fn functions take a count parameter, but they are only invoked with count=1, so we might want to simplify that too

I will test all these tomorrow

[hjelmn]

@ggouaillardet I agree. I think the count was used at some point in the past. I agree about the reduce vs allreduce. Will make that change.

[bosilca]

I don't think that using bcast on an inter-communicator is equivalent to using allgatherv. The result of this operation is that only one group will get the data.

[hjelmn]

@bosilca We first do a reduce on the intercommunicator. This gives the leaders (rank 0 in each comm) the result from the other group. Then we exchange the info between the leaders and do the reduction. At this point both leaders have the same result. They then bcast to their local groups.

[hjelmn]

@bosilca Just to be clear the ibcast is on the intra-communicator we keep with every inter-communicator.

[bosilca]

@hjelmn the bcast on a intercommunicator is not symmetric. Only one of the leaders (which is the process providing MPI_ROOT as the root) will broadcast it's data to the remote group.

[bosilca]

if the ibcast is done on a intra-communicator, then the result should be correct.

[bosilca]

If we have an intra-communicator then why all the pain to deal with the reduce on an inter-communicator ?

[hjelmn]

@bosilca Not sure why the code was so complicated before. Once we added the c_local_comm we should have updated the algorithm to do what this PR makes it do. This is much less complicated than we made it out to be :).

[hjelmn]

@ggouaillardet Nevermind on the not using reduce. I forgot that I had already changed the initial reduce to be on the intra-communicator. Made the change from allreduce->reduce.

[hjelmn]

Might go ahead an merge this before 9pm EDT to get it into MTT tonight.

[ggouaillardet]

It works great for me, thanks !

[rhc54]

I don't see any change in MTT, though - still a ton of hangs. However, the tests that are now hanging are comm_dup.

[ggouaillardet]

3b968ec fixes MPI_Comm_dup_c
(that was a separate issue, if it did not occur earlier it is probably because it was hidden by the bug fixed in this PR)
i am now running some other tests from the intel_tests suite, and see if i hit some more bugs

[rhc54]

just wondering - are these some of the same problems we are seeing in 2.x? I don't know if these fixes need to go over or not.

[ggouaillardet]

these two issues came from the refactor, and it was not brought to v2.x
i ll check v2.x from now, maybe something was backported and should have not ...

[ggouaillardet]

several hangs occur with the *_stride* collective tests from the ibm test suite. strictly speaking, these are not hangs since these tests complete in less than 10 minutes, but take more than 3 minutes so MTT consider them as hangs.

that should be fixed by open-mpi/ompi-tests@53ad6f22e1f9ceda580a06bf8e00324a5f15c6c0
(i simply #ifdef'ed out sanity checks that are time very consumming since they access strided data)

alltoallvw_zero from the ibm test suite (and maybe some more) will be fixed when open-mpi/ompi-release#1368 is merged