Add Missing MPI_F_XXX C constants

[drwootton]

Add missing MPI_F_XXX C constants to mpi.h
This resolves issue #10391

Signed-off-by: David Wootton dwootton@us.ibm.com

[hppritcha]

I'd suggest enhancing mpif-values.pl to write out a small 'c' include file in addition to the current *.h's for fortran, and add a line in mpi.h.in to include this small 'c' include file.
I think OMPI_FORTRAN_STATUS_SIZE is okay as is since its defined earlier in the generated mpi.h file.

[jsquyres]

@hppritcha I'm not sure I understand. Are you suggesting:

1. Have mpif-values.pl to write out (for example) mpif-values.c
2. Have mpi.h #include mpif-values.c
3. Install mpif-values.c into $(installdir) (so that it can be included by user apps who #include "mpi.h")

If so, could we try something else? I ask because C header files should be .h, not .c; installing a .c file into $(installdir) seems... weird. Also, we've tried hard over the years to keep mpi.h be a single, standalone file that doesn't include any other Open MPI files (aside from the C++ bindings, which, thankfully, are gone). Indeed, I think mpi.h currently #include's <stddef.h> so that we can get ptrdiff_t, but that's unavoidable.

[jsquyres]

I had a slightly-unconventional thought overnight. What about:

1. Put the hard-coded values of these 4 constants in configure.ac (or maybe ompi_setup_mpi_fortran.m4?)
2. Propagate the values down to mpif-values.pl and mpi.h from there.

This is a little unconventional because the goal of putting these values in configure.ac is not a) to test anything, or b) to make them configurable. But rather just so that these values are at the top of the substitution mechanism, so to speak. That way, the values can flow down to the relevant files via the normal via AC_DEFINE or AC_SUBST mechanisms.

There's probably some things that still need to be figured out here -- e.g., do we have to make mpif-values.pl.in? That seems undesirable. Perhaps mpif-values.pl can read a text config file to find those values, or somesuch...? (but be wary of VPATH builds!). TBD / up to the implementor.

[jjhursey]

I'm not following the need for the added complexity.

The new constants are meant to help a C application deal with the Fortran status structure which is defined in mpi.h.in:

ompi/ompi/include/mpi.h.in

Lines 480 to 488 in a43c46f

	struct ompi_f08_status_public_t {
	/* These fields are publicly defined in the MPI specification.
	User applications may freely read from these fields. */
	MPI_Fint MPI_SOURCE;
	MPI_Fint MPI_TAG;
	MPI_Fint MPI_ERROR;
	MPI_Fint internal[OMPI_FORTRAN_STATUS_SIZE - 3];
	};
	typedef struct ompi_f08_status_public_t ompi_f08_status_public_t;

MPI-4, pp. 844, ln. 5:

In C, such an f_status array can be defined with MPI_Fint f_status[ MPI_F_STATUS_SIZE]. Within this array, one can use in C the indexes MPI_F_SOURCE, MPI_F_TAG, and MPI_F_ERROR, to access the same elements as in Fortran with MPI_SOURCE, MPI_TAG and MPI_ERROR. The C indexes are 1 less than the corresponding indexes in Fortran due to the different default array start indexes in both languages.

So the new constants are only relevant to C programs, so should be part of mpi.h.in. Additionally, the MPI_F_SOURCE, MPI_F_TAG, MPI_F_ERROR are relative to their position in the structure defined in mpi.h.in. So I would assume that the defines are best placed as close to that structure as possible since they are essentially hard coded positional values for the fields of the structure. The other constant MPI_F_STATUS_SIZE is used in the ompi_f08_status_public_t padding and the value associated with it (OMPI_FORTRAN_STATUS_SIZE) is defined by configure earlier in mpi.h.in.

It is my understanding that mpif-values.pl is only for constant values that are to be used by Fortran. So these new constants would not qualify. Note that there are quite a few hard-coded, duplicated values from mpi.h.in and mpif-values.pl (e.g., MPI_UNDEFINED).

The only connection I see with the mpif-values.pl is in the last sentence from the MPI 4.0 text:

The C indexes are 1 less than the corresponding indexes in Fortran due to the different default array start indexes in both languages.

These values are indeed part of mpif-values.pl

ompi/ompi/include/mpif-values.pl

Lines 230 to 232 in a43c46f

	$constants->{MPI_SOURCE} = 1;
	$constants->{MPI_TAG} = 2;
	$constants->{MPI_ERROR} = 3;

So the corresponding C values need to be one less than these three values.

Suggestion - if my argument is correct:

• Keep the definitions of MPI_F_SOURCE, MPI_F_TAG, MPI_F_ERROR, MPI_F_STATUS_SIZE in mpi.h.in since they are intended for C programs.
• Add a comment above the new constants regarding their connection with other files in the repo. Something like the following:

/*
 * The constants MPI_F_SOURCE, MPI_F_TAG, MPI_F_ERROR, MPI_F_STATUS_SIZE
 * are defined relative to both their position in the MPI_Status structure and their
 * corresponding constant values in Fortran (i.e., MPI_SOURCE, MPI_TAG, MPI_ERROR).
 * The Fortran constants are defined in mpif-value.pl. The MPI_Status constant is defined
 * in this header file. Since these four MPI_F_ constants are meant for C programs
 * they are listed in this header file which also includes the MPI_Status structure declaration.
 */

[dalcinl]

I'm not following the need for the added complexity.

Just my two cents: I'm on @jjhursey side, I don't see the need to over engineer this.

If the order of the source/tag/error slots in MPI_Status is ever changed, keeping the MPI-4 indexes MPI_F_{SOURCE|TAG|ERROR} in sync requires much more than just updating integral values in a script. You will have to change the slot order in the ompi_[f08_]status_public_t structs as well.

Looks like the best course of action is to keep this simple, but I would still suggest to #define MPI_F_{SOURCE|TAG|ERROR} very close to the declaration of the two ompi_[f08_]status_public_t structures to minimize the risk of getting things out of sync. And if someone ever messes this up, the addition of some minimal testing should be more enough to catch the problem.

@jjhursey Note however that MPI_F_{SOURCE|TAG|ERROR} are indexes on an array of MPI_Fint, which is the Fortran 77 way to represent an MPI_Status. The ompi_f08_status_public_t struct you mention is the Fortran 2008 way. IIUC, from the MPI standard, the F77 and F08 ways do not necessarily have the same binary layout (although I find hard to believe an implementation would have to do that, unless dealing with a very unfriendly/pedantic Fortran compiler).

[jsquyres]

@jjhursey @dwootton-ny and I just chatted on a Webex about this.

Short version

I'll do 3 things:

1. Approve this PR as-is
2. Make another PR to amend/fix comments in mpi.h.in
3. File an issue to unify all integer constants between mpi.h.in and mpif-values.pl

In turn, @dwootton-ny will:

1. Get this PR merged
2. Since this is an MPI-3.1 errata item, make corresponding cherry-pick PRs for v4.0.x, v4.1.x, and v5.0.x

More detail

Here's a little more explanation...

My concern is that I didn't want to add numerical constants that do, in fact, have corresponding values in C and Fortran, but they're just hard-coded constants in mpi.h.in and mpif-values.pl, respectively. Instead, I'd prefer them to come from a common source (whatever that is), so that there's only a single source of truth for these integer values. Less chance for human error that way.

@jjhursey correctly made me understand that mpi.h.in is full of integer values that have the exact same integer values in mpif-values.pl -- i.e., we already have this implicit linkage problem with all the other, existing values: it's not unique to these 4 new values. Yep -- fair point.

So let's go ahead and get these 4 new values in (since it's an MPI-3.1 errata) and then have a separate issue/PR to do a proper unification of the integer constant values between mpi.h and the Fortran interfaces.

Per @dalcinl's comment, I'll also file another PR that adds a comment to mpi.h.in that says that if you edit any of the constants here in mpi.h.in, you should almost certainly also edit the corresponding values in mpif-values.pl. I'll also clean up a few existing comments that refer to old/stale Fortran header files (we found at least one -- I'll check the others). Might be worth adding some additional comments around status (per @dalcinl), too.

[jsquyres]

I'd suggest enhancing mpif-values.pl to write out a small 'c' include file in addition to the current *.h's for fortran, and add a line in mpi.h.in to include this small 'c' include file. I think OMPI_FORTRAN_STATUS_SIZE is okay as is since its defined earlier in the generated mpi.h file.

@hppritcha Can you re-review this PR based on the above comments? Your negative review is preventing this PR from being merged.

[jjhursey]

@drwootton Please cherry-pick to the v5.0.x, v4.1.x, and v4.0.x branches. Thanks!

[dalcinl]

@jsquyres @jjhursey @drwootton I'm confused. AFAICT, the MPI_F_{SOURCE|TAG|ERROR} constants were added in MPI-4. I don't see them mentioned in the MPI-3.1 errata. What am I missing? If I am correct, then why should this PR be cherry-picked to v4.1.x and v4.0.x?

[dalcinl]

@jsquyres I just noticed that this PR introduced trailing whitespace in this line. Maybe you can fix it in your upcoming PR.

[jsquyres]

We should have a CI check for that... Thanks for noticing; will fix in #10528.

[jjhursey]

Good point on the MPI 3.1. They were accepted into the v3 branch after v3.1 was release as errata. Since there is not a v3.2 (yet?) it is not finalized.

• Original email thread: https://lists.mpi-forum.org/pipermail/mpiwg-fortran/2020-September/003584.html
• MPI Forum Issue: MPI_STATUS_SIZE, MPI_SOURCE, MPI_TAG, MPI_ERROR not defined in C mpi-forum/mpi-issues#306
• MPI Forum PR for v4.0: https://github.com/mpi-forum/mpi-standard/pull/270
• MPI Forum PR for v3.x: https://github.com/mpi-forum/mpi-standard/pull/271

@drwootton So it only needs to be cherry-picked to the v5.0.x branch.

[jsquyres]

I don't think that's right. Errata are in force as soon as they are voted in. So technically, the MPI_F_* constants are "in" MPI-3 -- even though there's no new MPI-3 doc, and it looks like this item was erroneously left out of the MPI-3 errata doc (!).

I've talked to Wesley (MPI Forum Secretary) about this: he's going to follow up with Bill (MPI Forum Editor) and/or Martin (MPI Forum Grand Chief) and/or MPI_COMM_WORLD to make sure that the MPI-3 errata doc gets updated.

Bottom line: I still think that this should get cherry picked back to the OMPI 4.0.x and 4.1.x branches. 😄

[jsquyres]

@drwootton If everyone agrees and you make PRs for v4.0.x and v4.1.x, can you please include commit d44ec62 in those PRs? That will include the whitespace/comment fixes. Thanks!