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.

Sign up for GitHub

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

Jump to bottom

Implement MPI_IN_PLACE for MPI Operations #232

Merged
merged 35 commits into from
Feb 7, 2019
Merged

Implement MPI_IN_PLACE for MPI Operations #232

merged 35 commits into from
Feb 7, 2019

Conversation

PhilipVinc
Copy link
Contributor

@PhilipVinc PhilipVinc commented Jan 22, 2019 •
edited
Loading

(This is a fixed #230.)

Hi,

I would like to implement non allocating versions for several MPI operations, as well as support for MPI_IN_PLACE.

This PR defines a new variable, MPI.IN_PLACE of type Ptr{Cvoid}, and refactors the code of the following functions to implement a non-allocating version which accepts MPI.IN_PLACE if supported by MPI.

  • Allreduce
  • Allgather
  • Allgatherv
  • Alltoall
  • Alltoallv

For the following functions, calling MPI_IN_PLACE is slightly less straightforward, so I felt that the best way to support it would be to create a new method called XXX_in_place!.
If you think this is not a good idea, feel free to propose a better solution.

  • Scatter
  • Reduce
  • Gather
  • Gatherv

The last three functions listed above are still missing, and if you are interested I'll be glad to include them in the PR.

I believe all CI test failures on OpenMPI-MacOS and 32bit-Windows to be unrelated to the PR.

--
Filippo

@PhilipVinc PhilipVinc mentioned this pull request Jan 22, 2019
Closed
PhilipVinc
PhilipVinc commented Jan 22, 2019
View reviewed changes
src/MPI.jl Outdated Show resolved Hide resolved
@PhilipVinc
Copy link
Contributor Author

PhilipVinc commented Jan 23, 2019 •
edited
Loading

While I'm at it, I've got two very minor issues regarding the code:

  1. around line ~996 of mpi-base.jl there is the following definition
# allocate receive buffer automatically
function allreduce(sendbuf::MPIBuffertype{T}, op::Union{Op,Function}, comm::Comm) where T

The fact that it is underscore is intended or it's a typo?
For consistency I would like to rename it to Allreduce, and add a docstring specifying that it is allocating.

  1. The function definitions of Allreduce! and Reduce! that convert a user-provided function to MPI.Op are located in mpi-op.jl. For consistency, I would rather move them in mpi-base.jl. In fact, at first I had missed those two methods and it took me a moment to find where they were defined.

Do you agree with those two modifications?

@barche
Copy link
Collaborator

barche commented Jan 24, 2019

Yes, I agree. Scatter also has an allocating version, and it is starting with an uppercase letter.

barche
barche reviewed Jan 24, 2019
View reviewed changes
appveyor.yml Outdated Show resolved Hide resolved
@PhilipVinc
Revert "Change build rules"
e9338a2 
This reverts commit d814a5d.
@PhilipVinc
Allreduce:
9aed181 
 - Rename `allreduce(sendbuf, op, comm)` to `Allreduce(sendbuf, op, comm)` for consistency + fix test
 - Move `Allreduce!(send, recv, count op::Function, comm)` converting user provided-functions from `mpi-op.jl` to `mpi-base.jl`
@PhilipVinc
Docs for Allreduce
72a0376
@PhilipVinc
Reduce:
f56af06 
	- Add nonallocating version `Reduce!`
	- Move and rename `Reduce(send, recv, count op::Function, comm)` converting user provided-functions from `mpi-op.jl` to `mpi-base.jl`
	- Docs
@PhilipVinc
Add a deprecation warning to lowercase allreduce
b513833
@PhilipVinc
Copy link
Contributor Author

PhilipVinc commented Jan 24, 2019 •
edited
Loading

I renamed allreduce to Allreduce and added a deprecation warning for the lowercase version, so that if you merge this PR code will not be broken.

@PhilipVinc
Copy link
Contributor Author

@barche, @vchuravy I finished including all the functions that I had listed. The PR should now be complete.

@barche
Copy link
Collaborator

barche commented Jan 29, 2019

Sorry for the delay. To me this looks good to merge, anyone else want to review?

@PhilipVinc
Copy link
Contributor Author

No problems.
I noticed that MPI.Scatterv was missing from my initial list, so I gave it the same treatment.

I took the liberty of editing the README to document the allocating and the non-allocating versions of the various communications, and their support for IN_PLACE.

Feel free to edit/delete it if you don't like my changes.

@barche
Copy link
Collaborator

barche commented Feb 5, 2019

Since there were no more comments on this I think it's ready to merge. Should the commits be squashed into one, or is it OK like this?

@PhilipVinc
Copy link
Contributor Author

If you want me to squash the commits or do anything just let me know.

@barche barche merged commit bc2e979 into JuliaParallel:master Feb 7, 2019
@barche
Copy link
Collaborator

barche commented Feb 7, 2019

OK, I used the "squash and merge" button, which mostly worked except that it created an extra empty commit, sorry for that but I guess it's now carved in stone. Anyway, many thanks for this big PR!

@PhilipVinc PhilipVinc mentioned this pull request Feb 7, 2019
Closed
@PhilipVinc
Copy link
Contributor Author

PhilipVinc commented Feb 7, 2019 •
edited
Loading

You welcome! And thanks for accepting it.
For the sake of cleaning up, I believe that you can close the (quite old) PRs 86 and 157 as those features were part of my Pull Request.

@PhilipVinc PhilipVinc deleted the philipvinc/in_place branch February 7, 2019 23:18
This was referenced Feb 8, 2019
Closed
Closed
samo-lin pushed a commit to samo-lin/MPI.jl that referenced this pull request Jun 17, 2019
@PhilipVinc @barche
Implement MPI_IN_PLACE for MPI Operations (JuliaParallel#232)
73936bb 
* Extract the value of MPI_IN_PLACE during build phase
* Add non allocating version of MPI_Allreduce, support for MPI_IN_PLACE, and tests.
* Add non allocating version of MPI_Scatter, another method supporting MPI_IN_PLACE, and tests.
* Add non allocating version of MPI_Allgather, support for MPI_IN_PLACE, and tests.
* Add non allocating version of MPI_Allgatherv, support for MPI_IN_PLACE, and tests.
* Add non allocating version of MPI_Alltoall, support for MPI_IN_PLACE, and tests.
* Add non allocating version of Alltoallv and tests
* Modify definition of IN_PLACE and revert function signatures to the old type.
* Make the compiler check if a function can accept ConstantPtr.
* Allreduce:
 - Rename `allreduce(sendbuf, op, comm)` to `Allreduce(sendbuf, op, comm)` for consistency + fix test
 - Move `Allreduce!(send, recv, count op::Function, comm)` converting user provided-functions from `mpi-op.jl` to `mpi-base.jl`
* Reduce:
	- Add nonallocating version `Reduce!`
	- Move and rename `Reduce(send, recv, count op::Function, comm)` converting user provided-functions from `mpi-op.jl` to `mpi-base.jl`
* Add Reduce_in_place! function
* Modify allreduce tests to test N-dimensional tensors
* Gather: Add a non-allocating version
* Add Gather_in_place!
* Add non-allocating Gatherv and support for MPI.IN_PLACE + tests
* Add Gatherv_in_place!
* Add non-allocating Scatterv! and Scatterv_in_place
@simonbyrne simonbyrne mentioned this pull request Dec 16, 2019
Merged
@PhilipVinc PhilipVinc mentioned this pull request Jan 10, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@barche barche barche left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@PhilipVinc @barche