Skip to content

remove C wrapper of MPI_Allreduce_scalar #87

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
Merged
merged 2 commits into from
Apr 2, 2025
Merged

remove C wrapper of MPI_Allreduce_scalar #87

merged 2 commits into from
Apr 2, 2025

Conversation

gxyd
Copy link
Collaborator

@gxyd gxyd commented Apr 2, 2025

No description provided.

gxyd
gxyd commented Apr 2, 2025
View reviewed changes
src/mpi.f90 Outdated
type(c_ptr) :: sendbuf_ptr, recvbuf_ptr, c_datatype, c_op, c_comm
integer(c_int) :: local_ierr

sendbuf_ptr = c_loc(sendbuf)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this doesn't handle the case when sendbuf is sent as MPI_IN_PLACE

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this definitely needs handling of MPI_IN_PLACE, without which POT3D will run into an infinite loop.

@gxyd
Copy link
Collaborator Author

gxyd commented Apr 2, 2025

Thanks @adit4443ya for handling MPI_IN_PLACE. Now, I think we should be able to simplify it for MPI_Allreduce for other fortran subroutines as well.

@adit4443ya
Copy link
Collaborator

Yes !!, Let's Hope CI passes

@gxyd
Copy link
Collaborator Author

gxyd commented Apr 2, 2025

Yes, would you like to do that remaining work with this PR? That'll be great.

@gxyd
Copy link
Collaborator Author

gxyd commented Apr 2, 2025

i.e. to remove other variants of MPI_Allreduce from mpi_c_bindings.f90 and collate them into a single function. Hence, removing the MPI_Allreduce C wrapper at the end.

@adit4443ya
Copy link
Collaborator

Yes i will do that
But there are total four subroutines

MPI_Allreduce_scalar  !Done in this PR
MPI_Allreduce_1d
MPI_Allreduce_array_int
MPI_Allreduce_array_real

Scalar is being handled in this PR,
So just to make it simpler (and not clutter it) i will make other subsequent PRs for each of the cases

@adit4443ya
Copy link
Collaborator

Let me know how that sounds?

@gxyd gxyd merged commit 463ff93 into main Apr 2, 2025
20 checks passed
@gxyd gxyd deleted the allreduce_int_scalar_wrap_c branch April 2, 2025 13:41
@gxyd
Copy link
Collaborator Author

gxyd commented Apr 2, 2025

Perfect, I merged this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
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
@gxyd @adit4443ya