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.

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

MPI_F08 support for the MPL interface. #21

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

marsdeno
Copy link
Collaborator

  • legacy f77 interface can be enabled via MPL_F77_DEPRECATED ecbuild option

* legacy f77 interface can be enabled via MPL_F77_DEPRECATED ecbuild option
@marsdeno marsdeno requested review from wdeconinck and prgillies May 30, 2024 08:53
@FussyDuck
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@marsdeno marsdeno marked this pull request as draft May 30, 2024 08:53
Copy link
Contributor

@pmarguinaud pmarguinaud left a comment

Choose a reason for hiding this comment

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

I am not an expert at all in the latest MPI standard, so I cannot discuss the pros and cons of MPI 2008 vs the traditional F77 MPI.

Regarding the changes in the interfaces, I agree they are so small that we can switch directly to MPL with MPI 2008 in ARPEGE/IFS. The code of lfidiff.F90 has been commented out. This is a problem for us, so please restore it, and do not compile it if it is a problem for you.

I would like to have the checks on non-contiguous arrays to remain as a run time option. I agree that this has a cost, and probably has to be disabled for reaching full performance, but I think that keeping the ability to check for non-contiguous arrays entering the communications is something useful.

Eventually, I think it would be extremely profitable to rewrite some of these routines (in particular mpl_send/mpl_recv) with fypp, as the length of the code makes it very difficult to check for correctness.

IF(.NOT.PRESENT(KREQUEST)) CALL MPL_MESSAGE(KERROR,'MPL_SEND',' KREQUEST MISSING',LDABORT=LLABORT)
CALL MPI_IBCAST(PBUF(1,1),ICOUNT,INT(MPI_REAL4),IROOT-1,ICOMM,KREQUEST,IERROR)
IF(.NOT.PRESENT(KREQUEST)) CALL MPL_MESSAGE(CDMESSAGE='MPL_SEND',CDSTRING=' KREQUEST MISSING',KERROR=KERROR,LDABORT=LLABORT)
CALL MPI_IBCAST(PBUF(1,1),ICOUNT,MPI_REAL4,IROOT-1,ICOMM,IREQUEST_LOCAL,IERROR)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we pass PBUF(1,1) instead of PBUF, if MPI_F08 is supposed to handle non-contiguous arrays ?

@wdeconinck
Copy link
Collaborator

wdeconinck commented Jun 2, 2024

Hi @marsdeno I am very happy for this development. However this branch does not yet work with -DENABLE_MPI=OFF

fiat/src/fiat/mpl/internal/mpl_mpif.F90:12:5:

   12 | USE MPI_F08
      |     1
Fatal Error: Cannot open module file 'mpi_f08.mod' for reading at (1): No such file or directory
compilation terminated.

Normally it should then use the 'mpi_serial' dummy implementations.
So I believe what you need is an mpi_f08 wrapper for mpi_serial as well

And with -DENABLE_MPI=OFF -DENABLE_MPL_F77_DEPRECATED=ON:

[ 29%] Building Fortran object src/fiat/CMakeFiles/fiat.dir/util/ec_meminfo.F90.o
fiat/src/fiat/util/ec_meminfo.F90:115:15:

  115 | TYPE(MPI_COMM) :: ICOMM
      |               1
Error: Derived type 'mpi_comm' at (1) is being used before it is defined
fiat/src/fiat/util/ec_meminfo.F90:116:17:

  116 | TYPE(MPI_STATUS)   :: IRECV_STATUS
      |                 1
Error: Derived type 'mpi_status' at (1) is being used before it is defined
fiat/src/fiat/util/ec_meminfo.F90:141:7:

  141 | ICOMM%MPI_VAL = KCOMM
      |       1
Error: Symbol 'icomm' at (1) has no IMPLICIT type
fiat/src/fiat/util/ec_meminfo.F90:158:27:

  158 |    CALL MPI_COMM_RANK(ICOMM,MYPROC,ERROR)
      |                           1
Error: Symbol 'icomm' at (1) has no IMPLICIT type; did you mean 'kcomm'?
fiat/src/fiat/util/ec_meminfo.F90:268:83:

  268 |             CALL MPI_RECV(LASTNODE,LEN(LASTNODE),MPI_BYTE,I,ITAG,ICOMM,IRECV_STATUS,ERROR)
      |                                                                                   1
Error: Symbol 'irecv_status' at (1) has no IMPLICIT type

@marsdeno
Copy link
Collaborator Author

marsdeno commented Jun 4, 2024

@wdeconinck Indeed I will take care of these issues, thanks.

@marsdeno
Copy link
Collaborator Author

marsdeno commented Jun 4, 2024

I am not an expert at all in the latest MPI standard, so I cannot discuss the pros and cons of MPI 2008 vs the traditional F77 MPI.

Regarding the changes in the interfaces, I agree they are so small that we can switch directly to MPL with MPI 2008 in ARPEGE/IFS. The code of lfidiff.F90 has been commented out. This is a problem for us, so please restore it, and do not compile it if it is a problem for you.

I would like to have the checks on non-contiguous arrays to remain as a run time option. I agree that this has a cost, and probably has to be disabled for reaching full performance, but I think that keeping the ability to check for non-contiguous arrays entering the communications is something useful.

Eventually, I think it would be extremely profitable to rewrite some of these routines (in particular mpl_send/mpl_recv) with fypp, as the length of the code makes it very difficult to check for correctness.

Will do, regarding lfidiff.F90.
Noted, for the checks on contiguity. Would you think run time option is really preferable over compile time option?
And finally, I do agree on the fypp question, a lot of this code is a great use case for it. For a future PR :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants