Hi Jeff,
Jeffrey Squyres wrote:
>
> On Apr 3, 2012, at 10:56 PM, Kawashima wrote:
>
> > I and my coworkers checked mpi-f90-interfaces.h against MPI 2.2 standard
> > and found many bugs in it. Attached patches fix them for trunk.
> > Though some of them are trivial, others are not so trivial.
> > So I'll explain them below.
>
> Excellent -- many thanks for these!
>
> I have some notes on the specific patches, below, but first note the
> following: Craig Rasmussen and I have been working on revamping the Open MPI
> Fortran bindings for quite a while. Here's a quick summary of the changes:
>
> 1. two prototypes of the new MPI-3 mpi_f08 module
> a. Full implementation of all functions, not using F08 descriptors
> b. 6-function MPI using F08 descriptors (showing that it can be done) for
> ifort
> 2. for the existing mpi module:
> a. New implementation using "ignore TKR" directives
> b. If your fortran compiler doesn't support "ignore TKR" directives (e.g.,
> gfortran), fall back and use the old mpi module implementation
> 3. wrapper compiler changes
> a. New "mpifort" wrapper compiler; all Fortran interfaces are available
> b. mpif77 and mpif90 are sym links to mpifort (and may disappear someday)
>
> All of this work is available in a public bitbucket here:
>
> https://bitbucket.org/jsquyres/mpi3-fortran
Great. I'll see it later.
> There's a linker error in there at the tip at the moment; Craig is working on
> fixing it. When that's done, we'll likely put out a final public test
> tarball, and assuming that goes well, merge all this stuff into the OMPI SVN
> trunk. The SVN merge will likely be a little disruptive because the
> directory structure of the Fortran bindings changed a bit (e.g., all 5
> implementations are now under ompi/mpi/fortran).
>
> The point is that I plan to bring in all your fixes to this bitbucket branch
> so that all the new stuff and all your fixes come in to the trunk at the same
> time.
>
> 1.4 is dead; I doubt we'll be applying your fixes there.
>
> 1.5 has transitioned to 1.6 (yesterday); I can look into making a patch for
> the v1.6 series. The tricky part is preserving the mpi module ABI between
> 1.5.5 and 1.6. We've done this before, though, so I think it'll be do-able.
Thanks for your explanation.
I know we are preparing v1.6 and v1.4 is not active.
> > 1. incorrect parameter types
> >
> > Two trivial parameter type mismatches.
> > Fixed in my mpi-f90-interface.type-mismatch.patch.
> >
> > MPI_Cart_map periods: integer -> logical
> > MPI_Reduce_scatter recvcounts: missing "dimension(*)"
>
> Applied, and also made corresponding fixes to my ignore TKR mpi module (the
> f08 module didn't have these issues).
>
> > 2. incorrect intent against MPI 2.2 standard
> >
> > This is a somewhat complex issue.
> > First, I'll cite MPI 2.2 standard below.
> >
> > 2.3 in MPI 2.2 standard says:
> > There is one special case - if an argument is a handle to an opaque
> > object (these terms are defined in Section 2.5.1), and the object is
> > updated by the procedure call, then the argument is marked INOUT or OUT.
> > It is marked this way even though the handle itself is not modified -
> > we use the INOUT or OUT attribute to denote that what the handle
> > references is updated. Thus, in C++, IN arguments are usually either
> > references or pointers to const objects.
> >
> > 2.3 in MPI 2.2 standard also says:
> > MPI's use of IN, OUT and INOUT is intended to indicate to the user
> > how an argument is to be used, but does not provide a rigorous
> > classification that can be translated directly into all language
> > bindings (e.g., INTENT in Fortran 90 bindings or const in C bindings).
> > For instance, the "constant" MPI_BOTTOM can usually be passed to
> > OUT buffer arguments. Similarly, MPI_STATUS_IGNORE can be passed as
> > the OUT status argument.
> >
> > 16.2.4 in MPI 2.2 standard says:
> > Advice to implementors.
> > The appropriate INTENT may be different from what is given in the
> > MPI generic interface. Implementations must choose INTENT so that
> > the function adheres to the MPI standard.
> >
> > Hmm. intent in mpi-f90-interfaces.h does not necessarily match
> > IN/OUT/INOUT in MPI 2.2, especially regarding opaque objects.
> > mpi-f90-interfaces.h seems to have consideration of opaque objects
> > partially, which is handled in r9922 and r23098.
> >
> > I compared MPI 2.2 ("standard") and mpi-f90-interfaces.h ("header")
> > and classified problematic parameters into four types.
> >
> > Type A. Trivial error.
> >
> > These are fixed in my mpi-f90-interface.intent-error-a.patch to match
> > the standard.
> >
> > subroutine | parameter | standard | header
> > -----------------------------+-------------+----------+--------
> > MPI_Bcast | buffer | inout | in
>
> Bcast is a little tricky. I think the real solution is to remove the intent
> altogether from the buffer argument. We talked about this issue a LOT in the
> MPI-3 Fortran WG.
>
> Here's the short version:
>
> - intent(in): obvious
> - intent(out): the compiler is actually allowed to zero the buffer (!), which
> we clearly don't want
> - intent(inout): ...there is a reason that this is not desirable, but I don't
> remember what it is :-(
> - no intent at all: this one is safe
Oh, I didn't know intent(out) is so dangerous.
Thanks for your explanation.
> I honestly don't remember why intent(inout) is not good here, but there is
> very definitely a reason.
>
> *** Craig -- do you remember?
>
> However, I hadn't back-ported the removal of the intent to the mpi modules
> because we had intended to leave those alone as much as possible when
> implementing the mpi_f08 and ignore-TKR-style mpi module. But since you
> raise this issue, I think you're right -- the fix should be back-ported. So
> I did.
>
> > MPI_Pack | outsize | in | out
> > MPI_Comm_test_inter | comm | in | inout
> > | flag | out | in
> > MPI_Add_error_class | errorclass | out | in
> > MPI_Win_create | win | out | in
>
> Applied all of these.
>
> > MPI_Get | origin_addr | out | in
>
> MPI_Get is actually very much like MPI_Irecv -- it asynchronously fills the
> buffer (it *may* be filled in when you return, but it may not). So you could
> make it intent(out), but intent(out) may also make some fortran compilers
> zero out the buffer first, which we don't want to do. So just like we did
> for MPI_Irecv in mpi_f08, I removed the intent here.
>
> More specifically, we removed the "intent" clause from all OUT choice
> buffers, and for all asynchronously-filled choice buffers in the mpi_f08
> module. I'm thinking that we should probably do the same for the OMPI's 2
> implementations of the mpi module.
I agree. Removing intent seems best for us.
> > Type B. Not match but not error (opaque object).
> >
> > Though these parameters in the header don't match the standard,
> > it's OK because these are opaque object and should not be INOUT
> > for Fortran. Some of these are fixed in r9922 and r23098.
> > These should not be changed.
> >
> > subroutine | parameter | standard | header
> > -----------------------------+-------------+----------+--------
> > MPI_Comm_set_attr | comm | inout | in
> > MPI_Win_set_attr | win | inout | in
> > MPI_Type_set_attr | type | inout | in
> > MPI_Comm_set_errhandler | comm | inout | in
> > MPI_Win_set_errhandler | win | inout | in
> > MPI_File_set_errhandler | file | inout | in
> > MPI_File_set_view | fh | inout | in
> > MPI_Attr_put | comm | inout | in
> > MPI_Attr_delete | comm | inout | in
> > MPI_Errhandler_set | comm | inout | in
>
> I agree: these all look ok to me. No change necessary.
>
> > Type C. Not match and yet error (opaque object).
> >
> > This parameter is similar to Type B but it should be IN, not OUT.
> > This is fixed in my mpi-f90-interface.intent-error-c.patch to match
> > intent that it should be.
> >
> > subroutine | parameter | standard | header | should be
> > -----------------------------+-------------+----------+--------+-----------
> > MPI_Info_delete | info | inout | out | in
>
> Applied to both the TKR and ignore TKR mpi modules.
>
> > Type D. Match but error (opaque object).
> >
> > Though these parameters in the header match the standard,
> > they should be IN for Fortran because these are opaque objects and
> > their handles themselves are not changed.
> > These are fixed in my mpi-f90-interface.intent-error-d.patch to match
> > intent that they should be for Fortran.
> >
> > subroutine | parameter | standard | header | should be
> > -----------------------------+-------------+----------+--------+-----------
> > MPI_Comm_delete_attr | comm | inout | inout | in
> > MPI_Win_delete_attr | win | inout | inout | in
> > MPI_Type_delete_attr | type | inout | inout | in
> > MPI_Comm_set_name | comm | inout | inout | in
> > MPI_Type_set_name | type | inout | inout | in
> > MPI_Win_set_name | win | inout | inout | in
> > MPI_Info_set | info | inout | inout | in
> > MPI_Grequest_complete | request | inout | inout | in
> > MPI_File_set_size | fh | inout | inout | in
> > MPI_File_preallocate | fh | inout | inout | in
> > MPI_File_set_info | fh | inout | inout | in
> > MPI_File_write_at | fh | inout | inout | in
> > MPI_File_write_at_all | fh | inout | inout | in
> > MPI_File_iwrite_at | fh | inout | inout | in
> > MPI_File_read | fh | inout | inout | in
> > MPI_File_read_all | fh | inout | inout | in
> > MPI_File_write | fh | inout | inout | in
> > MPI_File_write_all | fh | inout | inout | in
> > MPI_File_iread | fh | inout | inout | in
> > MPI_File_iwrite | fh | inout | inout | in
> > MPI_File_seek | fh | inout | inout | in
> > MPI_File_read_shared | fh | inout | inout | in
> > MPI_File_write_shared | fh | inout | inout | in
> > MPI_File_iread_shared | fh | inout | inout | in
> > MPI_File_iwrite_shared | fh | inout | inout | in
> > MPI_File_read_ordered | fh | inout | inout | in
> > MPI_File_write_ordered | fh | inout | inout | in
> > MPI_File_seek_shared | fh | inout | inout | in
> > MPI_File_write_at_all_begin | fh | inout | inout | in
> > MPI_File_write_at_all_end | fh | inout | inout | in
> > MPI_File_read_all_begin | fh | inout | inout | in
> > MPI_File_read_all_end | fh | inout | inout | in
> > MPI_File_write_all_begin | fh | inout | inout | in
> > MPI_File_write_all_end | fh | inout | inout | in
> > MPI_File_read_ordered_begin | fh | inout | inout | in
> > MPI_File_read_ordered_end | fh | inout | inout | in
> > MPI_File_write_ordered_begin | fh | inout | inout | in
> > MPI_File_write_ordered_end | fh | inout | inout | in
> > MPI_File_set_atomicity | fh | inout | inout | in
> > MPI_File_sync | fh | inout | inout | in
>
> Some of patch d failed to apply; I *think* because those interfaces were
> already updated (but I didn't check -- I only checked that the end result was
> correct). I updated the ignore TKR implementation, too.
I'll check it in bitbucket.
> The mpi_f08 implementation didn't have this issue; it had the intents correct
> already.
>
> > 3. incorrect intent for MPI_IN_PLACE
> >
> > 5.2.1 in MPI 2.2 standard says:
> > Advice to users.
> > By allowing the "in place" option, the receive buffer in many of the
> > collective calls becomes a send-and-receive buffer. For this reason,
> > a Fortran binding that includes INTENT must mark these as INOUT,
> > not OUT. Note that MPI_IN_PLACE is a special kind of value; it has
> > the same restrictions on its use that MPI_BOTTOM has.
> >
> > My mpi-f90-interfaces.intent-inplace.patch adapt intent parameters
> > to match intent for Fortran regarding MPI_IN_PLACE.
> > Note that intent of MPI_Scatter(v) should not be changed because
> > MPI_IN_PLACE can be specified for recvbuf instead of sendbuf and
> > no data are modified in sendbuf.
> >
> > subroutine | parameter | standard | header | should be
> > -----------------------------+-----------+----------+--------+-----------
> > MPI_Gather(v) | recvbuf | out | out | inout
> > MPI_Scatter(v) | sendbuf | in | in | in
> > MPI_Allgather(v) | recvbuf | out | out | inout
> > MPI_Alltoall(v,w) | recvbuf | out | out | inout
> > MPI_Reduce | recvbuf | out | out | inout
> > MPI_Allreduce | recvbuf | out | out | inout
> > MPI_Reduce_scatter | recvbuf | out | out | inout
> > MPI_Scan | recvbuf | out | out | inout
> > MPI_Exscan | recvbuf | out | out | inout
>
> Per the above general rule, we should remove the intent for all OUT
> parameters. I *think* that the intent(in) should be ok for Scatter(v).
I agree.
> > It seems that MPI_Reduce_scatter_block, which appears in MPI 2.2, is not
> > implemented in trunk yet. So my patch doesn't include modification for it.
>
> Correct; we haven't implemented yet. We're waiting for ORNL's new collective
> improvements (which, as I understand it, includes reduce_scatter_block).
>
> > 4. mismatched interface and bridge
> >
> > I compared ompi/mpi/f90/scripts/mpi-f90-interfaces.h ("interface") and
> > ompi/mpi/f90/scripts/mpi_*_f90.f90.sh ("bridge") and found some mismatches
> > between them. The interfaces (except MPI_Mrecv) seems to be modifed in
> > r11057 but (I imagine) the bridges are forgotten to be modified at that
> > time.
> > These are fixed in my mpi-f90-interfaces.mismatched-bridge.patch to match
> > the interface.
> >
> > subroutine | parameter | interface | bridge
> > -----------------------------+-----------+-----------+--------
> > MPI_Recv | status | out | inout
> > MPI_Sendrecv | status | out | inout
> > MPI_Sendrecv_replace | status | out | inout
> > MPI_Mrecv | status | out | inout
>
> Per above, I think the real solution is to remove the intent for the OUT
> params.
I agree.
> I've pushed all these changes to my bitbucket -- could you double check what
> I pushed? I can make you a tarball if that would be easier than grabbing my
> mercurial tree.
Thanks, no tarball needed. I'll check them in bitbucket in a few days.
Regards,
Takahiro Kawashima,
MPI development team,
Fujitsu
