Boost C++ Libraries: Ticket #6704: Boost version of mpi_in_place

attachment set

Mon, 19 Mar 2012 10:33:52 GMT

attachment → allreduce.cpp

c++ source code

Mon, 19 Mar 2012 10:39:59 GMT

One solution could be to transparently test for equality of the two buffers and silently pass MPI_IN_PLACE to MPI_Allreduce, another could to provide a boost mpi_in_place object with automatic conversion to T*. The first one is probably a no-brainner for user, but then, something explicit in MPI is made implicit in boost mpi. I can provide the patch, other operation could be concerned.

Regards,

Mon, 19 Mar 2012 11:00:48 GMT

Another issue with th e implicit solution is that the input buffer is not left unchanged, while it is documented as such. So I guess a acceptable solution would be to simply provide a global:

namespace boost{ namespace mpi {
template<typename T> T const* mpi_in_place(T const* p = 0) { return static_cast<T const*>(MPI_IN_PLACE); }
} }

function ?

attachment set

Tue, 20 Mar 2012 10:20:33 GMT

attachment → mpi_in_place_tck6704_rev77427.patch

A proposed patch for the missing (?) feature)

Tue, 20 Mar 2012 10:24:46 GMT

The attached path propose a fix for this issue. It add the 'missing' all_reduce prototypes with only one input/output buffer. When appropriate, it is implemented by passing MPI_IN_PLACE to the existing boost implementations as a sendbuffer. When it is not possible (user types and user op), it re-introduces the missing buffer and uses a reduce/broadcast combination. The all_reduce test have been updated to check for in place and out of place array all_reduce operations.

Please let me know if there is a problem wit this patch.

Regards

Tue, 20 Mar 2012 10:28:37 GMT

Note that since the new prototype are just an override of existing prototype with only one parameter, the existing documentation probably still holds.

Matthias Troyer — Thu, 11 Oct 2012 09:27:35 GMT

I think the nicest would be a version that just takes a single inout value.

anonymous — Thu, 11 Oct 2012 10:00:29 GMT

Replying to troyer:

I think the nicest would be a version that just takes a single inout value.

Agreed, but should we do something about the situation where the user explicitly provides the MPI_IN_PLACE argument ? Provided we let the code fail at runtime, should we throw an exception ?

Matthias Troyer — Thu, 11 Oct 2012 10:09:26 GMT

Why would the user want to provide it explicitly if we give a nicer interface to all_reduce that takes just a single inout parameter?

anonymous — Thu, 11 Oct 2012 12:10:43 GMT

Maybe someone coming from the C api.... but agreed, it's (probably too) far fetched.

I will go for the one inout param along with a clarification in the documentation then.

anonymous — Thu, 11 Oct 2012 13:51:10 GMT

One more question:

Right now, in the attached patch, we have

template<typename T, typename Op>
  void all_reduce(const communicator & comm, const T & in_value,
                  T & out_value, Op op);
template<typename T, typename Op>
  T all_reduce(const communicator & comm, const T & in_value, Op op);
template<typename T, typename Op>
  void all_reduce(const communicator & comm, const T * in_values, int n,
                  T * out_values, Op op);

plus

template<typename T, typename Op>
  void all_reduce(const communicator & comm, T * inout_values, int n,
                  Op op);

but we do not have:

template<typename T, typename Op>
  void all_reduce(const communicator & comm, T & in_value, Op op);

that's based on the assumption that there isn't much usage for it and that it would cause confusing overload issues.

I am still trying to find out how to modify the documentation :-)

attachment set

Thu, 11 Oct 2012 15:41:17 GMT

attachment → mpi_all_reduce_in_place.patch

updated patch for revision 80953

Thu, 11 Oct 2012 15:48:04 GMT

So, the attachment mpi_all_reduce_in_place.patch should implement the proposed modification. It include a inout version of the function for arrays, a modification of the doxygen documentation (what I was unable to test because I have issues with the doc generation) and updated test suite to exercise the feature.

Here is a summary of the context in which the patch was produced:

[alainm@login02 trunk]$ svn up
At revision 80953.
[alainm@login02 trunk]$ svn diff > mpi_all_reduce_in_place.patch
[alainm@login02 trunk]$ svn status -q
M       boost/mpi/collectives.hpp
M       boost/mpi/collectives/all_reduce.hpp
M       libs/mpi/test/all_reduce_test.cpp
M       libs/mpi/doc/mpi.qbk
[alainm@login02 trunk]$ svn info .
Path: .
URL: http://svn.boost.org/svn/boost/trunk
Repository Root: http://svn.boost.org/svn/boost
Repository UUID: b8fc166d-592f-0410-95f2-cb63ce0dd405
Revision: 80953
Node Kind: directory
Schedule: normal
Last Changed Author: viboes
Last Changed Rev: 80950
Last Changed Date: 2012-10-11 01:24:24 +0200 (Thu, 11 Oct 2012)
[alainm@login02 trunk]$

Pleae let me know if it's ok.

Thanks!

Thu, 11 Oct 2012 15:48:56 GMT

I forgot: it was tested with openmpi 1.6.1

Thu, 18 Oct 2012 08:39:23 GMT

Hi,

If something more need to be done before it can be commited, please let me know.

Regards

anonymous — Mon, 31 Dec 2012 11:39:10 GMT

Replying to anonymous:

One more question:

Right now, in the attached patch, we have

template<typename T, typename Op>
  void all_reduce(const communicator & comm, const T & in_value,
                  T & out_value, Op op);
template<typename T, typename Op>
  T all_reduce(const communicator & comm, const T & in_value, Op op);
template<typename T, typename Op>
  void all_reduce(const communicator & comm, const T * in_values, int n,
                  T * out_values, Op op);

plus

template<typename T, typename Op>
  void all_reduce(const communicator & comm, T * inout_values, int n,
                  Op op);

but we do not have:

template<typename T, typename Op>
  void all_reduce(const communicator & comm, T & in_value, Op op);

that's based on the assumption that there isn't much usage for it and that it would cause confusing overload issues.

After discussing this I find that actually an in-place version that does not return a copy makes more sense for in-place semantics, and the call might be as

template<typename T, typename Op>
void all_reduce(const communicator & comm, mpi::inplace_t<T> in_value, Op op);

where mpi::inplace_t<T> wrapper might be created by an mpi::inplace function. Thus by using mpi::inplace around the one argument we don't return anything. I would not use a two-parameter version where one parameter is replaced by an mpi::inplace placeholder like the C API does. We should go for a nicer C++ API and not try to just copy the C API as the deprecated official MPI C++ API did.

owner changed

Matthias Troyer — Mon, 31 Dec 2012 11:41:37 GMT

owner changed from Douglas Gregor to Matthias Troyer

attachment set

Wed, 16 Jan 2013 11:59:59 GMT

attachment → inplace6704-r82503.patch

patch with explicit inplace version of all reduce

Wed, 16 Jan 2013 12:04:35 GMT

in inplace6704-r82503.patch , you will find a patch with an explicit in place version of all_reduce. Both pointer and non pointer version requires the inplace wrapper.

A new file has been added, I did put my name in the copyright (I do not know the usage, feel free to modify).

alain.miniussi@… — Wed, 24 Jul 2013 09:57:03 GMT

Hi Matthias,

Is it ok for this one to go into trunk ?

Thanks,

Alain

Matthias Troyer — Wed, 24 Jul 2013 22:50:21 GMT

Can you please add documentation for this and a regression test? I will then commit it.

attachment set

alain.miniussi@… — Thu, 25 Jul 2013 13:18:22 GMT

attachment → mpi_all_red-r85156-6704.patch

patch with tests and documentation

anonymous — Thu, 25 Jul 2013 13:25:51 GMT

In the last attachment mpi_all_red-r85156-6704.patch , the doc has been updated (mostly the doxigen documentation) the regression test was already in [source:/trunk/libs/mpi/test/all_reduce_test.cpp] (I followed existing practice, that file now also test the new overload).

A typo has been fixed in the docbook, but it's been otherwise left untouched (since it's more an optimization than a functionnality, I did not think where was anything to add in there (?))

Let me know if it's ok.

Thanks!

Wed, 28 Aug 2013 13:30:17 GMT

It seems that the patch has not been applied, is there a pending issue with the fix ?

Best regards

Alain

Matthias Troyer — Wed, 28 Aug 2013 13:55:50 GMT

It would be good to explain it in the documentation, so that people know about it. Sorry for not being clear but I've been waiting for that.

attachment set

Thu, 29 Aug 2013 14:35:09 GMT

attachment → ticket6704-svn85498.patch.gz

patch with tests and documentation

Thu, 29 Aug 2013 14:40:15 GMT

ticket6704-svn85498.patch.gz contains the same path augmented with new documentation:

in mpi.qbk, an example has been added to illustrate the usage of all_reduce using the in place and out of place version and the possibility of optimization associated with MPI_IN_PLACE.
an entry as been added in the C-mapping section.

Let me know if this is what is expected.

As a side note, we might want to add a similar functionality to reduce at some point (?)

Regards