Skip to content

v4.0.x: fortran/use-mpi-f08: revamp mpi_f08 constants #7116

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Dec 13, 2019
Merged

v4.0.x: fortran/use-mpi-f08: revamp mpi_f08 constants #7116

merged 6 commits into from
Dec 13, 2019

Conversation

@ggouaillardet
Copy link
Contributor

@ggouaillardet ggouaillardet commented Oct 29, 2019

No description provided.

@ggouaillardet ggouaillardet added this to the v4.0.3 milestone Oct 29, 2019
@ggouaillardet ggouaillardet force-pushed the topic/v4.0.x/f08_bind_c_constants_revamp branch from 6c35cdb to e986f21 Compare October 29, 2019 08:05
@ggouaillardet
Copy link
Contributor Author

ggouaillardet commented Oct 29, 2019

@jsquyres can you please review this?

I incidentally noted some stuff is missing. For example type(MPI_Op) :: MPI_NO_OP is defined in the master branch but not in the v4.0.x branch. Is this something intentional ?

@jsquyres jsquyres changed the title fortran/use-mpi-f08: revamp mpi_f08 constants v4.0.x: fortran/use-mpi-f08: revamp mpi_f08 constants Oct 29, 2019
@jsquyres
Copy link
Member

jsquyres commented Oct 29, 2019

Have you tested that this doesn't break ABI w.r.t. all the constants changing types?

I can't think of a reason why MPI_NO_OP wouldn't be here on v4.0.x. I wonder if that's a prior mistaken conflict resolution on a cherry pick to v4.0.x, or something along those lines...?

@kawashima-fj
Copy link
Member

kawashima-fj commented Nov 1, 2019

@ggouaillardet I added MPI_NO_OP of mpi_f08 in #6174 but probablly forgot to cherry-pick it to release branches. Can you checy-pick (or manually add without an additional commit) it to this PR?

@hppritcha
Copy link
Member

hppritcha commented Nov 1, 2019

Why do we want this in v4.0.x? Aside from the discussion about missing MPI_NO_OP, is it just to get the f08 bindings to work with the old llvm fortran front end? It does look like #6174 should be pulled in however.

@jsquyres
Copy link
Member

jsquyres commented Nov 4, 2019

@hppritcha This is in reference to #7091 (i.e., bug fix for the ARM complier).

@hppritcha
Copy link
Member

hppritcha commented Nov 5, 2019

@shamisp what's allinea's opinion of this flang problem?

@jsquyres
Copy link
Member

jsquyres commented Nov 5, 2019

@ggouaillardet We talked about this yesterday on the RM call. Seems like it's two issues:

  1. MPI_NO_OP: That should definitely come over to v4.0.x.
  2. The flang fixes: Do we know if this is desired and/or necessary on the v4.0.x branch? I.e., we know it's a problem for flang, but does anyone care (given that -- at least as we understand it -- flang is old / may not be used much longer)?
  3. Do we know for sure that the changes don't impact ABI? I.e., did you test with an app compiled against v4.0.x and then try to run it with this PR? And what happens if we have some objects compiled with v4.0.x and some objects compiled with this PR -- does it all work properly (i.e., can an MPI_COMM_WORLD from one of the old v4.0.0 objects be passed to an object compiled with this PR)?

ggouaillardet and others added 6 commits November 6, 2019 09:48
@ggouaillardet
fortran/use-mpi-f08: add MPI C types
628883b 
Refers open-mpi#5801

Signed-off-by: Gilles Gouaillardet <gilles@rist.or.jp>

(cherry picked from commit open-mpi/ompi@69f1a19)
@kawashima-fj @ggouaillardet
fortran/use-mpi-f08: Remove unnecessary ;
792b5a0 
Signed-off-by: KAWASHIMA Takahiro <t-kawashima@jp.fujitsu.com>

(cherry picked from commit open-mpi/ompi@e0c5bad)
@kawashima-fj @ggouaillardet
fortran/use-mpi-f08: Add C++ datatypes and MPI_NO_OP
56d2865 
Though the MPI standard does not have `MPI_CXX_COMPLEX`, `mpi.h`,
`mpif.h`, and `mpi.mod` have it. So I added it for consistency.

Signed-off-by: KAWASHIMA Takahiro <t-kawashima@jp.fujitsu.com>

(cherry picked from commit open-mpi/ompi@63ecf01)
@ggouaillardet
fortran/use-mpi-f08: revamp constant declarations
23ed2f4 
In order to work around an issue with flang based compilers,
avoid declaring bind(C) constants and use plain Fortran parameter
instead.

For example,
type(MPI_Comm), bind(C, name="ompi_f08_mpi_comm_world") OMPI_PROTECTED :: MPI_COMM_WORLD
is changed to
type(MPI_Comm), parameter :: MPI_COMM_WORLD = MPI_Comm(OMPI_MPI_COMM_WORLD)

Note that in order to preserve ABI compatibility, ompi/mpi/fortran/use-mpi-f08/constants.{c,h}
have been kept even if its symbols are no more referenced by Open MPI.

Refs. open-mpi#7091

Signed-off-by: Gilles Gouaillardet <gilles@rist.or.jp>

(back-ported from commit open-mpi/ompi@b10a60a)
@ggouaillardet
fortran/use-mpi-f08: remove unused references to OMPI_PROTECTED
0ab61c9 
Signed-off-by: Gilles Gouaillardet <gilles@rist.or.jp>

(back-ported from commit open-mpi/ompi@df6d763)
@ggouaillardet
fortran/use-mpi-f08: misc fixes
02c79ac 
 - fix typos from open-mpi/ompi@b10a60a
 - remove remaining references to OMPI_PROTECTED from open-mpi/ompi@df6d763

Signed-off-by: Gilles Gouaillardet <gilles@rist.or.jp>

(cherry picked from commit open-mpi/ompi@fda4d04)
@ggouaillardet ggouaillardet force-pushed the topic/v4.0.x/f08_bind_c_constants_revamp branch from e986f21 to 02c79ac Compare November 6, 2019 01:18
@ggouaillardet
Copy link
Contributor Author

ggouaillardet commented Nov 6, 2019

  1. yes, and I also backported fortran/use-mpi-f08: add MPI C types #5811 (it seems it has been forgotten a while ago)

  2. I have some hard time undertanding this argument.

  • armflang and maybe PGI are flang based
  • f18 is still relying on pgf90 to work

so yes, the plan is to move away from flang and use f18 instead, but we are not quite there, so I'd rather revamp Open MPI (assuming it is impact free) than waiting an unknown amount of time for flang, ARM and PGI to fix their compilers.

  1. the plan was to be ABI backward compatible, but I will run some more tests from now.

@ggouaillardet
Copy link
Contributor Author

ggouaillardet commented Nov 7, 2019

I made some tests and found no ABI issues.

@jsquyres
Copy link
Member

jsquyres commented Nov 7, 2019

Ok, good to know. The discussion on the Monday+Tuesday Webexes was that flang was no longer relevant; if that's inaccurate, then it does change the equation.

@shamisp Can someone from ARM please chime in here?

@ggouaillardet
Copy link
Contributor Author

ggouaillardet commented Nov 27, 2019

I discussed flang vs f18 at SC, and I can confirm f18 is not quite yet ready to replace flang (f18 currently requires pgf90 to work). Also, armflang and pgf90 are currently based on flang, and we can expect it will take some extra time before they rebase on top of f18. Last but not least, we have no guarantee that f18 will behave differently from flang on that matter.
My best bet is to move forward is merge this PR.
I can also PR vs the v3.1.x release branch if needed.

@gpaulsen
Copy link
Member

gpaulsen commented Dec 12, 2019

@hppritcha I'm ready to merge this to v4.0.x as @ggouaillardet has tested this for ABI compatibility issues, and found none.

@hppritcha hppritcha merged commit 59d8d62 into open-mpi:v4.0.x Dec 13, 2019
@gpaulsen gpaulsen mentioned this pull request Apr 13, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jsquyres jsquyres jsquyres approved these changes

Assignees

@jsquyres jsquyres

Labels

enhancement fortran bindings Target: v4.0.x

Projects

None yet

Milestone

v4.0.3

Development

Successfully merging this pull request may close these issues.

5 participants

@ggouaillardet @jsquyres @kawashima-fj @hppritcha @gpaulsen