Skip to content

Add Missing MPI_F_XXX C constants #10491

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
merged 1 commit into from
Jun 28, 2022
Merged

Add Missing MPI_F_XXX C constants #10491

merged 1 commit into from
Jun 28, 2022

Conversation

drwootton
Copy link
Contributor

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

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

Signed-off-by: David Wootton <dwootton@us.ibm.com>
@awlauria awlauria requested review from hppritcha and jsquyres June 21, 2022 14:53
Copy link
Member

@hppritcha hppritcha left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member

jsquyres commented Jun 22, 2022

@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
Copy link
Member

jsquyres commented Jun 23, 2022

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
Copy link
Member

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

$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
Copy link
Contributor

dalcinl commented Jun 28, 2022

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
Copy link
Member

@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
Copy link
Member

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 jjhursey merged commit 9ef007e into open-mpi:main Jun 28, 2022
@jjhursey
Copy link
Member

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

@dalcinl
Copy link
Contributor

dalcinl commented Jun 29, 2022

@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?

* Constants for C code to access elements in Fortran MPI status array.
*/
#define MPI_F_STATUS_SIZE OMPI_FORTRAN_STATUS_SIZE /* Size of Fortran MPI status array */
#define MPI_F_SOURCE 0 /* Index for MPI_SOURCE */
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

@jjhursey
Copy link
Member

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.

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

@jsquyres
Copy link
Member

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
Copy link
Member

@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!

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.

5 participants