Corrections to Fortran 2008 interfaces #5453

PhilippOtte · 2018-07-18T14:08:51Z

Following changes were carried out in ompi/mpi/fortran/use-mpi-f08:

corrected the intent of the send buffer on most signatures in the Fortran 2008 interface
added the ASYNCHRONOUS attribute to send and receive buffers and other array dummy arguments in non-blocking collective calls (also in mod/mpi-f08-interfaces.F90)
corrected attributes in the C bindings defined in mpi-f-interfaces-bind.h

Important: I was not able to compile other parts of the code and could not check whether files compile but I think it would be wasted effort not to create a PR

Refs #5442

ompiteam-bot · 2018-07-18T14:08:53Z

Can one of the admins verify this patch?

jsquyres · 2018-07-18T14:20:02Z

ok to test

jsquyres · 2018-07-18T14:25:56Z

@PhilippOtte Thanks for the PR! Please note that we have a "Signed off by" requirement on our commits to indicate that you agree to our contributors declaration: https://github.com/open-mpi/ompi/blob/master/.github/CONTRIBUTING.md#contributors-declaration

jsquyres · 2018-07-18T14:28:23Z

ompi/mpi/fortran/use-mpi-f08/mpi-f08.F90

 ! The sizeof interfaces

-  include "sizeof_f08.h"
+!  include "sizeof_f08.h"


Did you mean to comment this out? That doesn't seem correct.

ibm-ompi · 2018-07-18T14:36:58Z

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

Gist: https://gist.github.com/1d987ba950d7a00a542a6a6e1fa75a58

ggouaillardet · 2018-07-18T14:37:41Z

ompi/mpi/fortran/use-mpi-f08/mpi-f-interfaces-bind.h

   BIND(C, name="ompi_iallgather_f")
   implicit none
-   OMPI_FORTRAN_IGNORE_TKR_TYPE, INTENT(IN) :: sendbuf, recvbuf
+   OMPI_FORTRAN_IGNORE_TKR_TYPE, INTENT(IN), ASYNCHRONOUS :: sendbuf, recvbuf


sendbuf and recvbuf cannot be defined twice

I saw, that too. I'm also getting a compile error that I'm puzzling over:

EDIT: See next comment.

Oops -- I originally pasted in the wrong compile error. This is the right one:

PPFC ialltoallw_f08.lo ialltoallw_f08.F90:35:53: call ompi_ialltoallw_f(sendbuf,sendcounts,sdispls,sendtypes(1)%MPI_VAL,& 1 Error: Rank mismatch in argument ‘sendtypes’ at (1) (rank-1 and scalar)

Ah, I see the problem. Directed comment coming up...

jsquyres · 2018-07-18T14:49:36Z

ompi/mpi/fortran/use-mpi-f08/mpi-f-interfaces-bind.h

+   OMPI_FORTRAN_IGNORE_TKR_TYPE, ASYNCHRONOUS :: recvbuf
+   INTEGER, INTENT(IN), ASYNCHRONOUS :: sendcounts(*), sdispls(*), recvcounts(*), rdispls(*)
+   INTEGER, INTENT(IN), ASYNCHRONOUS :: sendtypes(*)
+   INTEGER, INTENT(IN), ASYNCHRONOUS :: recvtypes(*)


You changed the types of sendtypes and recvtypes from scalars to arrays here. They were specifically scalars (in both ompi_alltoallw_f and ompi_ialltoallw_f) -- see the lengthy comment here: https://github.com/open-mpi/ompi/blob/master/ompi/mpi/fortran/use-mpi-f08/ialltoallw_f08.F90#L24-L33

Yes, it's a hack, but there did not seem to be another performant way to get the values from one call to the other (i.e., without allocating a temporary array from the heap [because this is ialltoallw, and we'll return before sendtypes/recvtypes are finished being used], individually copying all the elements from sendtypes/recvtypes to the temporary array, and then passing the temporary array to the underlying call. Then there will need to be some kind of hook when the request completes to know to free that temporary array, too).

Is there a better way?

Fortran allows you to obtain the address of the first element in an array via the intrinsic c_loc which returns a type(c_ptr) which is equivalent to a void pointer in C. There would be no copying involved and you would prevent the compiler from doing any unwanted magic there.

Can you add that to this PR? I'm wasn't enough of a Fortran expert to get that to work back when I originally wrote this stuff -- this hack was the best that I could come up with. 😳

According to Modern Fortran explained by Metcalf, Reid and Cohen the proposed way of using c_loc may lead to invalid pointers. So, your hack is probably the safer way of doing things.

PhilippOtte · 2018-07-18T14:58:52Z

Is there a safe way of signing off the unsigned commits or should I create a new branch with the changes in it and create a new PR?

ibm-ompi · 2018-07-18T15:08:48Z

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

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

jsquyres · 2018-07-18T15:22:23Z

You can git rebase -i origin/master to edit the existing commits on this PR. You'll then need to force-push back up to your branch (because you changed git history) -- that's just about the only socially-acceptable time to git-force-push.

PhilippOtte · 2018-07-30T07:25:46Z

@ggouaillardet do you still request changes?

jsquyres

You should probably squash the last 3 commits into the earlier commit(s). I.e., there's no real point in having a PR that makes change X in a commit, and then in a later commit on the same PR, removes change X.

jsquyres · 2018-08-02T21:21:53Z

Sorry -- meant to say: squash the last 2 commits.

jsquyres

Gah -- this PR fell off my radar (because github doesn't mail us upon force pushes). Doh! This all looks good to me now. @ggouaillardet can you review? (you have a negative review on this PR, but I think everything is fixed now)

jsquyres · 2018-09-24T19:21:44Z

@ggouaillardet Can you re-review? All the problems have been fixed.

gpaulsen · 2018-09-24T19:25:22Z

@ggouaillardet Can you please re-review?

gpaulsen · 2018-09-24T19:26:02Z

@PhilippOtte Can you please rebase (again)?

jsquyres · 2018-09-24T19:36:42Z

I just rebased -- we're now good. Just need a new review from @ggouaillardet

gpaulsen · 2018-09-24T21:18:45Z

Question, is this going to break binary compatibility with the older compiled apps? Even if this is more correct, if it breaks binary compatibility, this might trigger a new major release version bump.

jsquyres · 2018-09-24T21:21:07Z

I'm not going to swear to it, but I don't think INTENT affects the resulting symbol.

A Fortran expert should chime in here...

(too funny: Firefox underlines Fortran like it's misspelled! 😆)

ggouaillardet · 2018-09-25T00:28:02Z

the same changes have to be applied to ompi/mpi/fortran/use-mpi-f08/profile too. I will prepare a PR from now.

Should this PR make it for v4.0.0 ?

Corrected the signatures of the collectives used by the Fortran 2008 interface to state correct intent for inout arguments and use the ASYNCHRONOUS attribute in non-blocking collective calls. Also corrected the C-bindings in Fortran accordingly Signed-off-by: Philipp Otte <philipp.j.otte@googlemail.com>

Corrected the signatures of the collectives used by the Fortran 2008 interface to state correct intent for inout arguments and use the ASYNCHRONOUS attribute in non-blocking collective calls. Signed-off-by: Gilles Gouaillardet <gilles@rist.or.jp>

ggouaillardet · 2018-09-25T03:42:13Z

@jsquyres I fixed the signatures for the PMPI subroutines and fixed MPI_Ireduce_scatter[_block]() parameters too. As far as I am concerned, this is good for primetime.

gpaulsen · 2018-09-27T13:19:00Z

First round of answers from Fortran experts here say:

As far as I know, the intent attribute does not change the calling convention and the signature of the procedure. It should not cause binary incompatibilities.

ggouaillardet · 2018-10-08T13:37:38Z

@gpaulsen is this something stated in the standard ? I might be a bit too paranoid/pedantic here, but unless stated in the standard, we should theoretically verify this technical choice was made on all supported compilers.

jsquyres reviewed Jul 18, 2018

View reviewed changes

ggouaillardet requested changes Jul 18, 2018

View reviewed changes

jsquyres reviewed Jul 18, 2018

View reviewed changes

jsquyres added enhancement ⚠️ WIP-DNM! labels Jul 18, 2018

PhilippOtte force-pushed the corrections_F08_signatures_collectives branch from 7f4db0b to f2398a8 Compare July 19, 2018 08:36

bwbarrett added the Target: main label Jul 24, 2018

jsquyres requested changes Aug 2, 2018

View reviewed changes

PhilippOtte force-pushed the corrections_F08_signatures_collectives branch from f2398a8 to 4d92c89 Compare August 3, 2018 08:43

PhilippOtte force-pushed the corrections_F08_signatures_collectives branch from 4d92c89 to be6bb2a Compare August 28, 2018 13:46

jsquyres approved these changes Sep 14, 2018

View reviewed changes

jsquyres added bug Severity: critical NEWS and removed ⚠️ WIP-DNM! enhancement labels Sep 14, 2018

jsquyres force-pushed the corrections_F08_signatures_collectives branch from be6bb2a to d632969 Compare September 24, 2018 19:36

PhilippOtte and others added 2 commits September 25, 2018 11:16

ggouaillardet force-pushed the corrections_F08_signatures_collectives branch from d632969 to f750c69 Compare September 25, 2018 02:18

ggouaillardet approved these changes Sep 25, 2018

View reviewed changes

jsquyres approved these changes Sep 25, 2018

View reviewed changes

jsquyres merged commit 16e0ced into open-mpi:master Sep 25, 2018

jsquyres mentioned this pull request Sep 26, 2018

Incorrect intent on Fortran 08 interfaces to MPI_All* routines #5442

Closed

This was referenced Sep 27, 2018

v3.0.x: Fortran 08 bindings fixes #5788

Merged

v3.1.x: Fortran 08 bindings fixes #5781

Merged

v4.0.x: Fortran 08 bindings fixes #5780

Merged

Corrections to Fortran 2008 interfaces #5453

Corrections to Fortran 2008 interfaces #5453

Uh oh!

Conversation

PhilippOtte commented Jul 18, 2018 • edited by jsquyres Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ompiteam-bot commented Jul 18, 2018

Uh oh!

jsquyres commented Jul 18, 2018

Uh oh!

jsquyres commented Jul 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ibm-ompi commented Jul 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsquyres Jul 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PhilippOtte commented Jul 18, 2018

Uh oh!

ibm-ompi commented Jul 18, 2018

Uh oh!

jsquyres commented Jul 18, 2018

Uh oh!

PhilippOtte commented Jul 30, 2018

Uh oh!

jsquyres left a comment

Choose a reason for hiding this comment

Uh oh!

jsquyres commented Aug 2, 2018

Uh oh!

jsquyres left a comment

Choose a reason for hiding this comment

Uh oh!

jsquyres commented Sep 24, 2018

Uh oh!

gpaulsen commented Sep 24, 2018

Uh oh!

gpaulsen commented Sep 24, 2018

Uh oh!

jsquyres commented Sep 24, 2018

Uh oh!

gpaulsen commented Sep 24, 2018

Uh oh!

jsquyres commented Sep 24, 2018

Uh oh!

ggouaillardet commented Sep 25, 2018

Uh oh!

ggouaillardet commented Sep 25, 2018

Uh oh!

gpaulsen commented Sep 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ggouaillardet commented Oct 8, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

PhilippOtte commented Jul 18, 2018 •

edited by jsquyres

Loading

jsquyres Jul 18, 2018 •

edited

Loading

gpaulsen commented Sep 27, 2018 •

edited

Loading