Hi Jeff, I've checked your code in bitbucket. Two types of error are found. I've attached the patch.
First one (ignore-tkr) seems to be an error by manual patching. Second one (tkr) seems that patch command could not apply my fixes because neighboring lines were modified in your code. Regards, Takahiro Kawashima, MPI development team, Fujitsu > 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.
mpi3-fortran-interfaces.patch
Description: Binary data
