-
Notifications
You must be signed in to change notification settings - Fork 901
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
Conversation
Signed-off-by: David Wootton <dwootton@us.ibm.com>
There was a problem hiding this 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.
@hppritcha I'm not sure I understand. Are you suggesting:
If so, could we try something else? I ask because C header files should be |
I had a slightly-unconventional thought overnight. What about:
This is a little unconventional because the goal of putting these values in There's probably some things that still need to be figured out here -- e.g., do we have to make |
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 Lines 480 to 488 in a43c46f
MPI-4, pp. 844, ln. 5:
So the new constants are only relevant to C programs, so should be part of It is my understanding that The only connection I see with the
These values are indeed part of ompi/ompi/include/mpif-values.pl Lines 230 to 232 in a43c46f
So the corresponding C values need to be one less than these three values. Suggestion - if my argument is correct:
/*
* 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.
*/ |
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 Looks like the best course of action is to keep this simple, but I would still suggest to #define @jjhursey Note however that |
@jjhursey @dwootton-ny and I just chatted on a Webex about this. Short versionI'll do 3 things:
In turn, @dwootton-ny will:
More detailHere'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 @jjhursey correctly made me understand that 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 Per @dalcinl's comment, I'll also file another PR that adds a comment to |
@hppritcha Can you re-review this PR based on the above comments? Your negative review is preventing this PR from being merged. |
@drwootton Please cherry-pick to the v5.0.x, v4.1.x, and v4.0.x branches. Thanks! |
@jsquyres @jjhursey @drwootton I'm confused. AFAICT, the |
* 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 */ |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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. |
I don't think that's right. Errata are in force as soon as they are voted in. So technically, the 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. 😄 |
@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! |
Add missing MPI_F_XXX C constants to mpi.h
This resolves issue #10391
Signed-off-by: David Wootton dwootton@us.ibm.com