Add missing MPI_Status conversion subroutines #7762

ggouaillardet · 2020-05-26T10:39:15Z

MPI_Status_c2f08()
MPI_Status_f082c()
MPI_Status_f082f()
MPI_Status_f2f08()

and the PMPI_* related subroutines

Refs. #1475

Signed-off-by: Gilles Gouaillardet [email protected]

jsquyres

Looks good, except it is missing the use-mpi-ignore-tkr and use-mpi-tkr versions.

ompi/mpi/c/status_c2f08.c

ggouaillardet · 2020-05-27T06:29:38Z

@jsquyres are you sure we need these subroutines in the use mpi bindings?
i mean, are the use mpi bindings supposed to be aware of of the type(MPI_Status) type from use mpi_f08?

jsquyres · 2020-05-27T20:48:15Z

MPI-3.1 Figure 17.1 states what functions need to be available in which languages:

For the Fortran subroutines, it's not specified that the Fortran routines would only be available in the mpi_f08 module, so therefore they must be available in all Fortran interfaces (mpif.h, mpi module, and mpi_f08 module).

ggouaillardet · 2020-05-28T00:28:49Z

@jsquyres That does make sense. And it also begs the question "how to declare type(MPI_Status) in both mpif.h and use mpi" ? Can you please help me answering this one?

jsquyres · 2020-05-28T00:45:01Z

Forgive me, my Fortran is a little rusty, but perhaps something like this?

use :: mpi_f08_types, only : MPI_Status
implicit none
include 'mpif.h'
! ...use type(MPI_Status) and integer array with size MPI_STATUS_SIZE

and

use :: mpi_f08_types, only : MPI_Status
use mpi
! ...use type(MPI_Status) and integer array with size MPI_STATUS_SIZE

jsquyres · 2020-05-28T00:48:55Z

Oh, you're asking a different question... not how to use it in the user application, but how do we define it in the mpi module and mpif.h itself. Gotcha.

I guess defining that type in mpi and mpif.h would have to be contingent upon supporting enough F08 in the compiler (I don't know the correct preprocessor macro we establish for that in configure.ac, but I'm sure we have one).

Meaning: I would imagine that mpif.h and the mpi modules would just conditionally include a definition for the MPI_Status type. Extra bonus points if that definition can be a standalone file that can be included in all relevant places -- mpif.h, mpi module, mpi_f08 module (vs. replicating the code in multiple places).

ggouaillardet · 2020-07-10T00:23:01Z

@jsquyres I started this PR a while ago, and did not finalize it, mainly because I still cannot make any sense of MPI_Status_f08tof() and friend with include 'mpif.h' and use mpi.

One option is to declare type(MPI_Status) in mpif.h and mpi.mod.
An other option would be do (and have the end users do)

use mpi_f08, only :: type(MPI_Status)
implicit none
include 'mpif.h'
call MPI_Status_f08tof(...)

Both options look fishy to me, and even if I do not challenge your literal interpretation of the MPI standard
(e.g. it does not say use mpi_f08 only, so we have to do it for all Fortran flavors), I would rather ask the MPI
Forum what the initial intent was, and to tell us how we should interpret the standard (and some guidelines on how to implement it if needed).

Refs. #7918 #7921

jsquyres · 2020-07-10T00:52:23Z

Both options look fishy to me, and even if I do not challenge your literal interpretation of the MPI standard
(e.g. it does not say use mpi_f08 only, so we have to do it for all Fortran flavors), I would rather ask the MPI
Forum what the initial intent was, and to tell us how we should interpret the standard (and some guidelines on how to implement it if needed).

Ok, I'll ask.

I note that MPI-3.1 does define the C type MPI_F08_status. So at least there's an obvious answer for the C side (which you already implemented).

I suspect that it means that TYPE(MPI_Status) is going to need to be in mpif.h and the mpi module (if the compiler supports it). I not sure if that's exactly what we intended when we wrote that text, but I think that's a consequence of the existing MPI-3.1 language.

jsquyres · 2020-07-10T10:21:46Z

Asked to the MPI Forum Fortran working group here: https://lists.mpi-forum.org/pipermail/mpiwg-fortran/2020-July/003554.html

jsquyres · 2020-07-16T19:37:25Z

@ggouaillardet We got an answer from the MPI Forum Fortran working group: the MPI_Status_f208() and MPI_Status_f082f() routines and the TYPE(MPI_Status) type only need to be available in mpi_f08 and mpi -- not in mpif.h.

This may take a little time to wind through the Forum, but I think their conclusions are fairly definitive. See mpi-forum/mpi-issues#298.

Can you update this PR to match?

ggouaillardet · 2020-07-17T05:40:21Z

@jsquyres will do.
What should we do if the use_mpif08 bindings cannot be built?

should we still define type(MPI_Status) in mpi.mod?
if ye, should we still declare the Fortran to/from Fortran2008 subroutines in mpi.mod ? if yes, what should they do?
(failure? no-op and success?)

jsquyres · 2020-07-18T13:02:13Z

@jsquyres will do.
What should we do if the use_mpif08 bindings cannot be built?

should we still define type(MPI_Status) in mpi.mod?

If we are able to, yes. I.e., this is part of the mpi bindings, so we might as well do our best to build the mpi bindings (even though it's not technically useful in the mpi bindings without the mpi_f08 bindings, it's still part of the mpi bindings).

if ye, should we still declare the Fortran to/from Fortran2008 subroutines in mpi.mod ? if yes, what should they do?
(failure? no-op and success?)

If it is possible, we should have the routines and they should do their actual operations. Might as well not special case it as much as possible -- e.g., the mpi_f08 module might not be built for some obscure reason, but doing the INTEGER<-->TYPE(MPI_Status) conversion may still be possible to do.

ibm-ompi · 2020-07-22T05:17:37Z

The IBM CI (XL) build failed! Please review the log, linked below.

Gist: https://gist.github.com/724834b0718fa83b4f9bffd3f85af160

ibm-ompi · 2020-07-22T05:24:55Z

The IBM CI (PGI) build failed! Please review the log, linked below.

Gist: https://gist.github.com/b7020ee901c2c6cef4a47a443a45de2c

ibm-ompi · 2020-07-22T06:02:46Z

The IBM CI (XL) build failed! Please review the log, linked below.

Gist: https://gist.github.com/e9d6bced07c397b5a5141bfdb275eeb5

ggouaillardet · 2020-07-23T01:46:44Z

:bot:retest

ibm-ompi · 2020-07-26T11:31:14Z

The IBM CI (PGI) build failed! Please review the log, linked below.

Gist: https://gist.github.com/538b3acbfdb70d854f891f2dadc6f0ba

ibm-ompi · 2020-07-26T11:55:04Z

The IBM CI (PGI) build failed! Please review the log, linked below.

Gist: https://gist.github.com/02bb6f209f9b447969dac8eb737c59c4

ibm-ompi · 2020-07-26T12:34:21Z

The IBM CI (PGI) build failed! Please review the log, linked below.

Gist: https://gist.github.com/bd62c351c1eb88b8e987d565ce8fdd24

ggouaillardet · 2020-07-26T14:57:34Z

@jsquyres

I had to revamp more stuff than anticipated.

Finally, type(MPI_Status) is defined in its own mpi_types module (located in ompi/mpi/fortran/use-mpi) and this module is used by use-mpi-tkr, use-mpi-ignore-tkr and use-mpi-f08 if the compiler provides enough support.

There are two remaining comments I have not addressed yet.

jsquyres · 2020-08-19T16:19:10Z

FYI: the MPI Forum errata regarding this issue passed: mpi-forum/mpi-issues#298

jsquyres

Just need a few very minor changes.

I think the code all checks out. I made a comprehensive set of tests to check all of this -- see open-mpi/ompi-tests-public#2.

config/ompi_setup_mpi_fortran.m4

ompi/mpi/c/status_c2f08.c

ompi/mpi/c/status_f082c.c

ompi/mpi/man/man3/MPI_Status_f082c.3in

ompi/mpi/man/man3/MPI_Status_f082f.3in

ggouaillardet · 2020-09-08T08:25:03Z

I updated the PR (but did not rewrite the man pages in markdown)

jsquyres · 2020-09-08T13:21:23Z

I updated the PR (but did not rewrite the man pages in markdown)

Yeah, fair enough -- I wasn't expecting you to re-write it in Markdown. ;-) Could you review open-mpi/ompi-tests-public#2?

ompi/mpi/man/man3/MPI_Status_f082f.3in

ompi/mpi/man/man3/MPI_Status_f082c.3in

jsquyres

@ggouaillardet I added a trivial commit to add {} and another commit to convert the 3 man pages to Markdown. I fixed some typos and slightly improved the text along the way. If you're good with the changes, go ahead and merge.

jsquyres · 2020-09-08T15:50:05Z

bot:aws:retest

jsquyres · 2020-09-09T13:56:41Z

Just to tally up before this PR is merged, this PR resulted in 3 issues being brought before the MPI Fourm 😄 :

Fortran errata: Clarify where TYPE(MPI_Status) type must be defined mpi-forum/mpi-issues#298
MPI_STATUS_SIZE, MPI_SOURCE, MPI_TAG, MPI_ERROR not defined in C mpi-forum/mpi-issues#306
C bindings for MPI_STATUS_F082F and MPI_STATUS_F2F08 are missing const mpi-forum/mpi-issues#307

That's pretty good! 😜

Make the C MPI_F08_status type definition match the updated mpi_f08 type(MPI_Status) definition. This fix the inconsistency introduced in open-mpi/ompi@98bc7af Signed-off-by: Gilles Gouaillardet <[email protected]>

manually cleanup the generated .mod file in OMPI_FORTRAN_CHECK_BIND_C_TYPE Signed-off-by: Gilles Gouaillardet <[email protected]>

Only in C bindings: - MPI_Status_c2f08() - MPI_Status_f082c() In all bindings but mpif.h - MPI_Status_f082f() - MPI_Status_f2f08() and the PMPI_* related subroutines As initially inteded by the MPI forum, the Fortran to/from Fortran 2008 conversion subtoutines are *not* implemented in the mpif.h bindings. See the discussion at mpi-forum/mpi-issues#298 Refs. open-mpi#1475 Signed-off-by: Gilles Gouaillardet <[email protected]>

Convert the MPI_Status_f082f, MPI_Status_f082c, and MPI_Status_f2c man pages to Markdown. Fix some typos and improve the text a bit along the way. Left the raw NROFF redirect pages MPI_Status_f2f08, MPI_Status_c2f08, and MPI_Status_c2f files as they were -- they're 1-line redirects, and it seems simpler to leave those (vs. duplicating the Markdown). Signed-off-by: Jeff Squyres <[email protected]>

markalle · 2020-09-09T19:00:30Z

Should there be Fortran versions for _c2f08 and _f082c? I may be missing them, but I only see Fortran versions for the _f2f08 and _f082f calls.

jsquyres · 2020-09-09T19:09:48Z

Should there be Fortran versions for _c2f08 and _f082c? I may be missing them, but I only see Fortran versions for the _f2f08 and _f082f calls.

No.

See #7762 (comment) (aka MPI-3.1 figure 17.1).

markalle · 2020-09-09T22:13:24Z

Ah perfect, thanks. I saw the comment but was totally misreading the figure.

ggouaillardet added fortran bindings Target: main labels May 26, 2020

ggouaillardet requested a review from jsquyres May 26, 2020 10:39

jsquyres requested changes May 26, 2020

View reviewed changes

ompi/mpi/c/status_c2f08.c Outdated Show resolved Hide resolved

ggouaillardet force-pushed the topic/mpi_status_f08_c branch from 1d5c8e9 to e6d5a08 Compare May 27, 2020 06:27

ggouaillardet requested a review from jsquyres May 27, 2020 06:47

awlauria added this to the v5.0.0 milestone Jun 30, 2020

awlauria added the Target: v5.0.x label Jun 30, 2020

jsquyres mentioned this pull request Jul 16, 2020

Fortran errata: Clarify where TYPE(MPI_Status) type must be defined mpi-forum/mpi-issues#298

Closed

ggouaillardet force-pushed the topic/mpi_status_f08_c branch from e6d5a08 to c6eafa9 Compare July 22, 2020 04:58

ggouaillardet force-pushed the topic/mpi_status_f08_c branch from c6eafa9 to 06a9aeb Compare July 22, 2020 05:45

ggouaillardet force-pushed the topic/mpi_status_f08_c branch 4 times, most recently from 6bc34ec to e7b903a Compare July 22, 2020 13:42

ggouaillardet force-pushed the topic/mpi_status_f08_c branch from 737d2fa to 0d0f629 Compare July 26, 2020 11:11

ggouaillardet force-pushed the topic/mpi_status_f08_c branch from 0d0f629 to 3f7d57c Compare July 26, 2020 11:34

ggouaillardet force-pushed the topic/mpi_status_f08_c branch from 3f7d57c to d85191b Compare July 26, 2020 12:13

ggouaillardet force-pushed the topic/mpi_status_f08_c branch from d85191b to 7edd0fd Compare July 26, 2020 12:41

jsquyres reviewed Sep 7, 2020

View reviewed changes

ggouaillardet force-pushed the topic/mpi_status_f08_c branch from 7edd0fd to 6488f16 Compare September 8, 2020 02:22

jsquyres reviewed Sep 8, 2020

View reviewed changes

ompi/mpi/man/man3/MPI_Status_f082f.3in Outdated Show resolved Hide resolved

jsquyres reviewed Sep 8, 2020

View reviewed changes

ompi/mpi/man/man3/MPI_Status_f082c.3in Outdated Show resolved Hide resolved

jsquyres approved these changes Sep 8, 2020

View reviewed changes

gpaulsen requested a review from markalle September 8, 2020 15:36

ggouaillardet and others added 4 commits September 9, 2020 06:59

update MPI_F08_status type

7fce2f3

Make the C MPI_F08_status type definition match the updated mpi_f08 type(MPI_Status) definition. This fix the inconsistency introduced in open-mpi/ompi@98bc7af Signed-off-by: Gilles Gouaillardet <[email protected]>

configury: cleanup .mod file

466a2b3

manually cleanup the generated .mod file in OMPI_FORTRAN_CHECK_BIND_C_TYPE Signed-off-by: Gilles Gouaillardet <[email protected]>

jsquyres force-pushed the topic/mpi_status_f08_c branch from cb15697 to c04dc35 Compare September 9, 2020 13:59

markalle approved these changes Sep 9, 2020

View reviewed changes

jsquyres merged commit 1b0dfcd into open-mpi:master Sep 10, 2020

jsquyres mentioned this pull request Sep 15, 2020

Bring over Fortran MPI_Status conversion function fixes to v4.1.x #8047

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add missing MPI_Status conversion subroutines #7762

Add missing MPI_Status conversion subroutines #7762

ggouaillardet commented May 26, 2020

jsquyres left a comment

ggouaillardet commented May 27, 2020

jsquyres commented May 27, 2020

ggouaillardet commented May 28, 2020

jsquyres commented May 28, 2020 •

edited

Loading

jsquyres commented May 28, 2020

ggouaillardet commented Jul 10, 2020

jsquyres commented Jul 10, 2020

jsquyres commented Jul 10, 2020

jsquyres commented Jul 16, 2020

ggouaillardet commented Jul 17, 2020

jsquyres commented Jul 18, 2020

ibm-ompi commented Jul 22, 2020

ibm-ompi commented Jul 22, 2020

ibm-ompi commented Jul 22, 2020

ggouaillardet commented Jul 23, 2020

ibm-ompi commented Jul 26, 2020

ibm-ompi commented Jul 26, 2020

ibm-ompi commented Jul 26, 2020

ggouaillardet commented Jul 26, 2020

jsquyres commented Aug 19, 2020 •

edited

Loading

jsquyres left a comment

ggouaillardet commented Sep 8, 2020

jsquyres commented Sep 8, 2020

jsquyres left a comment

jsquyres commented Sep 8, 2020

jsquyres commented Sep 9, 2020

markalle commented Sep 9, 2020

jsquyres commented Sep 9, 2020

markalle commented Sep 9, 2020

Add missing MPI_Status conversion subroutines #7762

Add missing MPI_Status conversion subroutines #7762

Conversation

ggouaillardet commented May 26, 2020

jsquyres left a comment

Choose a reason for hiding this comment

ggouaillardet commented May 27, 2020

jsquyres commented May 27, 2020

ggouaillardet commented May 28, 2020

jsquyres commented May 28, 2020 • edited Loading

jsquyres commented May 28, 2020

ggouaillardet commented Jul 10, 2020

jsquyres commented Jul 10, 2020

jsquyres commented Jul 10, 2020

jsquyres commented Jul 16, 2020

ggouaillardet commented Jul 17, 2020

jsquyres commented Jul 18, 2020

ibm-ompi commented Jul 22, 2020

ibm-ompi commented Jul 22, 2020

ibm-ompi commented Jul 22, 2020

ggouaillardet commented Jul 23, 2020

ibm-ompi commented Jul 26, 2020

ibm-ompi commented Jul 26, 2020

ibm-ompi commented Jul 26, 2020

ggouaillardet commented Jul 26, 2020

jsquyres commented Aug 19, 2020 • edited Loading

jsquyres left a comment

Choose a reason for hiding this comment

ggouaillardet commented Sep 8, 2020

jsquyres commented Sep 8, 2020

jsquyres left a comment

Choose a reason for hiding this comment

jsquyres commented Sep 8, 2020

jsquyres commented Sep 9, 2020

markalle commented Sep 9, 2020

jsquyres commented Sep 9, 2020

markalle commented Sep 9, 2020

jsquyres commented May 28, 2020 •

edited

Loading

jsquyres commented Aug 19, 2020 •

edited

Loading