Skip to content

use-mpi-f08: fix prototypes and more #13168

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.

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

Merged

Conversation

hppritcha
Copy link
Member

In the course of checking out a user reported problem:

https://www.mail-archive.com/[email protected]//msg35382.html

it was discovered that many of the f08 entry points were improperly treating 'c' int args to the internal ompi*_f functions as fortran integers.

This commit fixes that and also addressed the problem reported by
the user. It was a special case where they were building the fortran
bindings with non-default integer size which happened to no longer match
the 'c' int.

The fact that this problem wasn't seen before just shows that probably 99% of the time the default Fortran integer kind is compatible with 'c' int.

@hppritcha
Copy link
Member Author

For the record, the compiler flags with gnu to use to reproduce the user problem are

./configure  FFLAGS="-m64 -fdefault-integer-8" FCFLAGS="-m64 -fdefault-integer-8" CFLAGS=-m64 CXXFLAGS=-m64

@hppritcha
Copy link
Member Author

hppritcha commented Mar 25, 2025

i should point out that the fortran code generated by the python binding code in PR #12226 does this in a much more elegant manner but only for a few of the files touched in this PR that are impacted by big count. If time allows or I find an energetic intern, we could convert all of the remaining *.F90 files - even those not impacted by big count - to this approach and dispense with the coupling to the 'c' code in mpif-h.

Here's for instance how the code for MPI_Pack_external_size_f08 generated by the python code looks in that PR. Note no need to pass funny end of list of args 'c' ints:

subroutine MPI_Pack_external_size_f08(datarep,  &
                                      incount,  &
                                      datatype, &
                                      size,     &
                                      ierror)
    use :: iso_c_binding, only: c_char
    use :: mpi_f08_types, only: MPI_COUNT_KIND, MPI_Datatype, MPI_ADDRESS_KIND
    implicit none
    CHARACTER(LEN=*), INTENT(IN) :: datarep
    INTEGER, INTENT(IN) :: incount
    TYPE(MPI_Datatype), INTENT(IN) :: datatype
    INTEGER(KIND=MPI_ADDRESS_KIND), INTENT(OUT) :: size
    INTEGER, OPTIONAL, INTENT(OUT) :: ierror
    INTEGER :: c_ierr

    interface
        subroutine ompi_pack_external_size_wrapper_f08(datarep,  &
                                                       incount,  &
                                                       datatype, &
                                                       size,     &
                                                       ierror) &
            BIND(C, name="ompi_pack_external_size_wrapper_f08")
            use :: iso_c_binding, only: c_char
            use :: mpi_f08_types, only: MPI_COUNT_KIND, MPI_Datatype, MPI_ADDRESS_KIND
            implicit none
            CHARACTER(KIND=C_CHAR), INTENT(IN) :: datarep(*)
            INTEGER, INTENT(IN) :: incount
            INTEGER, INTENT(IN) :: datatype
            INTEGER(KIND=MPI_ADDRESS_KIND), INTENT(OUT) :: size
            INTEGER, INTENT(OUT) :: ierror
        end subroutine ompi_pack_external_size_wrapper_f08
    end interface

    call ompi_pack_external_size_wrapper_f08(datarep,          &
                                             incount,          &
                                             datatype%MPI_VAL, &
                                             size,             &
                                             c_ierr)
    if (present(ierror)) ierror = c_ierr
end subroutine MPI_Pack_external_size_f08

subroutine MPI_Pack_external_size_f08_c(datarep,  &
                                        incount,  &
                                        datatype, &
                                        size,     &
                                        ierror)
    use :: iso_c_binding, only: c_char
    use :: mpi_f08_types, only: MPI_COUNT_KIND, MPI_Datatype
    implicit none
    CHARACTER(LEN=*), INTENT(IN) :: datarep
    INTEGER(KIND=MPI_COUNT_KIND), INTENT(IN) :: incount
    TYPE(MPI_Datatype), INTENT(IN) :: datatype
    INTEGER(KIND=MPI_COUNT_KIND), INTENT(OUT) :: size
    INTEGER, OPTIONAL, INTENT(OUT) :: ierror
    INTEGER :: c_ierr

    interface
        subroutine ompi_pack_external_size_wrapper_f08_c(datarep,  &
                                                         incount,  &
                                                         datatype, &
                                                         size,     &
                                                         ierror) &
            BIND(C, name="ompi_pack_external_size_wrapper_f08_c")
            use :: iso_c_binding, only: c_char
            use :: mpi_f08_types, only: MPI_COUNT_KIND, MPI_Datatype
            implicit none
            CHARACTER(KIND=C_CHAR), INTENT(IN) :: datarep(*)
            INTEGER(KIND=MPI_COUNT_KIND), INTENT(IN) :: incount
            INTEGER, INTENT(IN) :: datatype
            INTEGER(KIND=MPI_COUNT_KIND), INTENT(OUT) :: size
            INTEGER, INTENT(OUT) :: ierror
        end subroutine ompi_pack_external_size_wrapper_f08_c
    end interface

    call ompi_pack_external_size_wrapper_f08_c(datarep,          &
                                               incount,          &
                                               datatype%MPI_VAL, &
                                               size,             &
                                               c_ierr)
    if (present(ierror)) ierror = c_ierr
end subroutine MPI_Pack_external_size_f08_c

@ggouaillardet
Copy link
Contributor

Looks good to me, kuddos for the clever use of LEN(...) to skip declaring a local variable!

@jsquyres
Copy link
Member

@hppritcha In the call to ompi_pack_external_size_wrapper_f08, how does the C routine receive the string length argument -- does it have an additional int argument at the end of the list?

Also, if the fortran code is compiled with -fdefault-integer-8, is the implicit string length argument 4 bytes or 8?

@jsquyres
Copy link
Member

clever use of LEN(...)

Agreed -- that's cool!

I notice that 4 files were deleted in this PR. Was that intentional?

@hppritcha
Copy link
Member Author

yes the deletion was intentional. they should not have been there. i vaguely recall these were also found and removed as part of #12226

@hppritcha
Copy link
Member Author

@hppritcha In the call to ompi_pack_external_size_wrapper_f08, how does the C routine receive the string length argument -- does it have an additional int argument at the end of the list?

Also, if the fortran code is compiled with -fdefault-integer-8, is the implicit string length argument 4 bytes or 8?

the 'c' routine doesn't need this pasted on string length arguments (generated by the compiler when it doesn't know language is of the invoked routine) because of the interface definition

           CHARACTER(KIND=C_CHAR), INTENT(IN) :: datarep(*)

the fortran compiler generates a 'c' null terminated string under the covers which gets passed to ompi_pack_external_size_wrapper_f08.

@hppritcha
Copy link
Member Author

add on to the above comment. Since the compiler (fortran one) generates a properly formatted 'c' string then there is no 4/8 byte length argument problem.

After we get this in, and i have some time, i'm going to check out replacing the old crufty 'c' code in mpif-h with something like this but adjusted for the mpif.h/use mpi integer args for handles and lack of optional args. obviously no big count for those interfaces.

jsquyres
jsquyres previously approved these changes Mar 27, 2025
Copy link
Member

@jsquyres jsquyres left a comment

Choose a reason for hiding this comment

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

TIL:

the fortran compiler generates a 'c' null terminated string under the covers

Cool!

@jsquyres
Copy link
Member

After we get this in, and i have some time, i'm going to check out replacing the old crufty 'c' code in mpif-h with something like this but adjusted for the mpif.h/use mpi integer args for handles and lack of optional args. obviously no big count for those interfaces.

Should this be part of the ABI effort this summer, and/or a reason to examine slurping in the MPI Forum JSON and/or Python companion library?

@hppritcha
Copy link
Member Author

we'll see. i don't want to overwhelm interns with too many projects.

In the course of checking out a user reported problem:

https://www.mail-archive.com/[email protected]//msg35382.html

it was discovered that many of the f08 entry points were improperly treating
'c' int args to the internal ompi*_f functions as fortran integers.

This commit fixes that and also addressed the problem reported by
the user.   It was a special case where they were building the fortran
bindings with non-default integer size which happened to no longer match
the 'c' int.

The fact that this problem wasn't seen before just shows that probably
99% of the time the default Fortran integer kind is compatible with 'c' int.

Signed-off-by: Howard Pritchard <[email protected]>
@hppritcha hppritcha force-pushed the fix_binding_issue_for_use_mpi_f08_funcs branch from e5cf026 to 99cffec Compare April 7, 2025 15:18
@hppritcha hppritcha requested a review from jsquyres April 7, 2025 15:20
@hppritcha
Copy link
Member Author

@jsquyres odd a rebase to fix conflicts necessitates a re-review I guess.

hppritcha added a commit to hppritcha/ompi that referenced this pull request Apr 7, 2025
A bug was found with the c templates for the handle
conversion routines comm_c2f and win_c2f while working on

open-mpi#13168

Signed-off-by: Howard Pritchard <[email protected]>
@hppritcha
Copy link
Member Author

@jsquyres when you have time could you do quick re-review? no changes since you last approved just a rebase on top of main.

@hppritcha hppritcha merged commit 32bfff1 into open-mpi:main Apr 9, 2025
15 checks passed
hppritcha added a commit to hppritcha/ompi that referenced this pull request Apr 16, 2025
This PR refactors some of the f08 template files introduced in PR open-mpi#12621.
Many of them had been using, incorrectly, macros from fint_2_int.h.
The code in the use-mpi-f08 folder now makes use of generated code
in the wrapper around the internal c code to avoid many of the problems
that these older macros were trying to solve by providing 'c' interfaces
directly to the fortran compiler.  Also, simpified macros for handling
translation of arrays of MPI_Fint's to c int's are used in these
templates.

Related to open-mpi#13168 and associated issue raised by a user on the mail list.

Signed-off-by: Howard Pritchard <[email protected]>
hppritcha added a commit to hppritcha/ompi that referenced this pull request Apr 21, 2025
This PR refactors some of the f08 template files introduced in PR open-mpi#12621.
Many of them had been using, incorrectly, macros from fint_2_int.h.
The code in the use-mpi-f08 folder now makes use of generated code
in the wrapper around the internal c code to avoid many of the problems
that these older macros were trying to solve by providing 'c' interfaces
directly to the fortran compiler.  Also, generalized macros for handling
translation of arrays of MPI_Fint's to c int's (and other types) are used in these
templates.

Related to open-mpi#13168 and associated issue raised by a user on the mail list.

Signed-off-by: Howard Pritchard <[email protected]>
hppritcha added a commit to hppritcha/ompi that referenced this pull request Apr 29, 2025
This PR refactors some of the f08 template files introduced in PR open-mpi#12621.
Many of them had been using, incorrectly, macros from fint_2_int.h.
The code in the use-mpi-f08 folder now makes use of generated code
in the wrapper around the internal c code to avoid many of the problems
that these older macros were trying to solve by providing 'c' interfaces
directly to the fortran compiler.  Also, generalized macros for handling
translation of arrays of MPI_Fint's to c int's (and other types) are used in these
templates.

Related to open-mpi#13168 and associated issue raised by a user on the mail list.

Signed-off-by: Howard Pritchard <[email protected]>
hppritcha added a commit to hppritcha/ompi that referenced this pull request Apr 30, 2025
This PR refactors some of the f08 template files introduced in PR open-mpi#12621.
Many of them had been using, incorrectly, macros from fint_2_int.h.
The code in the use-mpi-f08 folder now makes use of generated code
in the wrapper around the internal c code to avoid many of the problems
that these older macros were trying to solve by providing 'c' interfaces
directly to the fortran compiler.  Also, generalized macros for handling
translation of arrays of MPI_Fint's to c int's (and other types) are used in these
templates.

Related to open-mpi#13168 and associated issue raised by a user on the mail list.

Signed-off-by: Howard Pritchard <[email protected]>
hppritcha added a commit to hppritcha/ompi that referenced this pull request Apr 30, 2025
This PR refactors some of the f08 template files introduced in PR open-mpi#12621.
Many of them had been using, incorrectly, macros from fint_2_int.h.
The code in the use-mpi-f08 folder now makes use of generated code
in the wrapper around the internal c code to avoid many of the problems
that these older macros were trying to solve by providing 'c' interfaces
directly to the fortran compiler.  Also, generalized macros for handling
translation of arrays of MPI_Fint's to c int's (and other types) are used in these
templates.

Related to open-mpi#13168 and associated issue raised by a user on the mail list.

Signed-off-by: Howard Pritchard <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants