Fix call to MPI_RECV. #15

Closed
wants to merge 1 commit into
from

Projects

None yet

3 participants

@QuLogic
Member
QuLogic commented Jan 6, 2014

The status argument should be an array, not the first element of one.
This is only a problem when compiling against the MPI module, as it
contains stricter type-checking.

@QuLogic QuLogic Fix call to MPI_RECV.
The status argument should be an array, not the first element of one.
This is only a problem when compiling against the MPI module, as it
contains stricter type-checking.
c124ae0
@komatits
Member
komatits commented Jan 6, 2014

This only holds if the original call to MPI uses the first element of
the array; any other value (which is legal in Fortran calls, and often
used) would lead to a bug when making the modification below.

On 01/06/2014 07:43 AM, Elliott Sales de Andrade wrote:

The status argument should be an array, not the first element of one.
This is only a problem when compiling against the MPI module, as it
contains stricter type-checking.


    You can merge this Pull Request by running

git pull https://github.com/QuLogic/specfem2d mpi-bug

Or view, comment on, or merge it at:

#15

    Commit Summary


Reply to this email directly or view it on GitHub
#15.

Dimitri Komatitsch
CNRS Research Director (DR CNRS), Laboratory of Mechanics and Acoustics,
UPR 7051, Marseille, France http://komatitsch.free.fr

@jedbrown
jedbrown commented Jan 6, 2014

@komatits Note that the declaration is

integer, dimension(MPI_STATUS_SIZE) :: msg_status

and that the MPI Standard includes numerous examples with the usage in this PR. However, the status is not being used here, so you may as well use MPI_STATUS_IGNORE.

@komatits
Member
komatits commented Jan 6, 2014

I agree, and I have also seen this declaration in hundreds of MPI codes.
I am surprised to see it leads to problems (as mentioned by Elliott).

On 01/06/2014 01:41 PM, Jed Brown wrote:

@komatits https://github.com/komatits Note that the declaration is

|integer, dimension(MPI_STATUS_SIZE) :: msg_status
|

and that the MPI Standard includes numerous examples with the usage in
this PR. However, the status is not being used here, so you may as well
use |MPI_STATUS_IGNORE|.


Reply to this email directly or view it on GitHub
#15 (comment).

Dimitri Komatitsch
CNRS Research Director (DR CNRS), Laboratory of Mechanics and Acoustics,
UPR 7051, Marseille, France http://komatitsch.free.fr

@QuLogic
Member
QuLogic commented Jan 6, 2014

I agree, and I have also seen this declaration in hundreds of MPI codes. I am surprised to see it leads to problems (as mentioned by Elliott).

The declaration is correct. It is the usage in the subroutine call that is wrong. The only reason that msg_status(1) worked before is that Fortran passes arguments by reference. A reference to the first element of the array is the same as a reference to the array itself.

However, when using the MPI module, it is possible that the implementation uses explicit interfaces. If the explicit interface says that the subroutine takes an array, then the compiler will not compile it if you do not pass an array.

@komatits
Member
komatits commented Jan 6, 2014

Yes, but then we'll have problems in cases like:

call my_subroutine(my_array(12))

which is correct in Fortran (points to the location of the 12th item of
the array) but will not work with the interface you mention.
As you say it is only in the case of my_array(1) that we can switch to
my_array.
In some of the SPECFEM codes there are cases in which we do refer to
parts of arrays that do not start at 1. (not in MPI calls though I
guess, but I am not 100% sure; it could be that in the case of some MPI
buffers we point to something inside rather than at the beginning)

(in some of the Earth model routines of 3D_GLOBE we often do that)

Thanks
Dimitri.

On 01/06/2014 11:53 PM, Elliott Sales de Andrade wrote:

I agree, and I have also seen this declaration in hundreds of MPI
codes. I am surprised to see it leads to problems (as mentioned by
Elliott).

The declaration is correct. It is the usage in the subroutine call that
is wrong. The only reason it worked before is that Fortran passes
arguments by reference. A reference to the first element of the array is
the same as a reference to the array itself.

However, when using the MPI module, it is possible that the
implementation uses explicit interfaces. If the explicit interface says
that the subroutine takes an array, then the compiler will not compile
it if you do not pass an array.


Reply to this email directly or view it on GitHub
#15 (comment).

Dimitri Komatitsch
CNRS Research Director (DR CNRS), Laboratory of Mechanics and Acoustics,
UPR 7051, Marseille, France http://komatitsch.free.fr

@QuLogic
Member
QuLogic commented Jan 6, 2014

I am not aware of any part of the SPECFEM codes that use explicit interfaces.

@komatits
Member
komatits commented Jan 6, 2014

they are used for all the MPI calls, now that we switched from include
mpif.h to "use mpi" a few months ago. Back then we checked all the MPI
calls (in the three versions of the code) and they were fine. Thus we
are probably all set.

On 01/07/2014 12:22 AM, Elliott Sales de Andrade wrote:

I am not aware of any part of the SPECFEM codes that use explicit
interfaces.


Reply to this email directly or view it on GitHub
#15 (comment).

Dimitri Komatitsch
CNRS Research Director (DR CNRS), Laboratory of Mechanics and Acoustics,
UPR 7051, Marseille, France http://komatitsch.free.fr

@QuLogic
Member
QuLogic commented Jan 6, 2014

Ah, I think you are referring to the buffer arguments (e.g., the first argument to MPI_Recv). These are special because they're "pass any type", whereas the status argument is explicitly an array of size MPI_STATUS_SIZE.

@jedbrown
jedbrown commented Jan 7, 2014

@QuLogic Which MPI complains about this? There isn't a void * in Fortran 90 and both MPICH and Open MPI (at least the 1.6 series) do not provide interfaces for functions like MPI_Recv, presumably because it cannot be expressed.

If you want to place statuses in some nontrivial part of an array, you can use slice notation. But I maintain that it would be better to use MPI_STATUS_IGNORE here because you aren't using the information.

@QuLogic
Member
QuLogic commented Jan 7, 2014

@QuLogic Which MPI complains about this? There isn't a void * in Fortran 90 and both MPICH and Open MPI (at least the 1.6 series) do not provide interfaces for functions like MPI_Recv, presumably because it cannot be expressed.

Nothing complains about the buffers; I am just trying to understand why @komatits thinks this would be a problem elsewhere and buffers are places where the-middle-of-an-array is used.

I am compiling against OpenMPI 1.7.3 and it does provide an explicit interface. Yes, there is no such thing as a void* in Fortran, but there is such a thing as a generic interface, so it's not impossible to do.

@QuLogic
Member
QuLogic commented Jan 7, 2014

I am going to take @jedbrown's advice and use MPI_STATUS_IGNORE since the status is unused. Since there are several places where this can be done, it will be in my cleanup branch.

@QuLogic QuLogic closed this Jan 7, 2014
@jedbrown
jedbrown commented Jan 7, 2014

The "generic interfaces" are just generated specializations for a (non-comprehensive) list of predefined types. Those all still call an untyped MPI_Recv. In your Open MPI build directory, look at ompi/mpi/fortran/use-mpi-tkr/mpi-f90-interfaces.h and ompi/mpi/fortran/mpi_recv_f90.f90 to see what they are doing. It creates tons of stub functions (see nm -D libmpi_usempi.so) which results in a massive number of symbol relocations (bigger binaries, slower startup). A mediocre solution by any measure, and still no type tag matching like Clang provides for C.

@QuLogic
Member
QuLogic commented Jan 7, 2014

Yes, I am aware how generic interfaces work. You said (emphasis added):

There isn't a void * in Fortran 90 and both MPICH and Open MPI (at least the 1.6 series) do not provide interfaces for functions like MPI_Recv, presumably because it cannot be expressed.

I am only stating a manner in which it could be expressed to produce an interface. Whether they choose to use it or not is up to them, as I develop neither one.

@jedbrown
jedbrown commented Jan 7, 2014

That style of generics is broken for this purpose: it cannot compile examples from the MPI standard.

http://www.open-mpi.org/community/lists/users/2014/01/23327.php

I didn't notice that they tried to do this, but I'm glad you pointed it out. The compiler extensions type(*) or IGNORE_TKR provide void* in a reasonable form, though these are not part of any standard. I am not aware of a way to express what they want within any Fortran standard without exposing the user to c_ptr.

@QuLogic QuLogic deleted the QuLogic:mpi-bug branch Jan 7, 2014
@komatits
Member
komatits commented Jan 7, 2014

I agree that switching to MPI_STATUS_IGNORE when the status is not used
(i.e. at least in all blocking Recv if I remember correctly) is a very
good idea. For non blocking Irecv I think we use either MPI_TEST() or
MPI_PROBE() (I don't remember if they need that status array, i.e. what
flag or array they test or probe)

On 01/07/2014 01:03 AM, Jed Brown wrote:

@QuLogic https://github.com/QuLogic Which MPI complains about this?
There isn't a |void *| in Fortran 90 and both MPICH and Open MPI (at
least the 1.6 series) do not provide interfaces for functions like
|MPI_Recv|, presumably because it cannot be expressed.

If you want to place statuses in some nontrivial part of an array, you
can use slice notation. But I maintain that it would be better to use
|MPI_STATUS_IGNORE| here because you aren't using the information.


Reply to this email directly or view it on GitHub
#15 (comment).

Dimitri Komatitsch
CNRS Research Director (DR CNRS), Laboratory of Mechanics and Acoustics,
UPR 7051, Marseille, France http://komatitsch.free.fr

@komatits
Member
komatits commented Jan 7, 2014

Great idea.

(independently) Do we have two diverging branches again? Let us try to
avoid that. We lost the whole month of August merging two diverging
versions of GLOBE, thus let us be more careful this time.

On 01/07/2014 01:47 AM, Elliott Sales de Andrade wrote:

I am going to take @jedbrown https://github.com/jedbrown's advice and
use |MPI_STATUS_IGNORE| since the status is unused. Since there are
several places where this can be done, it will be in my cleanup branch
https://github.com/QuLogic/specfem2d/tree/cleanup.


Reply to this email directly or view it on GitHub
#15 (comment).

Dimitri Komatitsch
CNRS Research Director (DR CNRS), Laboratory of Mechanics and Acoustics,
UPR 7051, Marseille, France http://komatitsch.free.fr

@QuLogic
Member
QuLogic commented Jan 7, 2014

That style of generics is broken for this purpose: it cannot compile examples from the MPI standard.

Ohh, you're referring to general OpenMPI usage; yes that could be a problem there. I was only talking about SPECFEM codes where I don't think we use anything exotic for the void* arguments.

(independently) Do we have two diverging branches again? Let us try to avoid that. We lost the whole month of August merging two diverging versions of GLOBE, thus let us be more careful this time.

It's in my fork. I need to check a few more things and then will open a PR. As I stated somewhere else, branches are not as big of a deal in git as in svn.

@QuLogic QuLogic referenced this pull request Jan 8, 2014
Merged

Cleanup several things. #17

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment