Nathan, I read your code and it's definitively looking good. I have however few minor issues with your patch.
1. MPI_Aint is unsigned as it must represent the difference between two memory arbitrary locations. In your MPI_Type_get_[true_]extent_x you go through size_t possibly reducing it's extent. I would suggest you used ssize_t instead. 2. In several other locations size_t is used as a conversion base. In some of these location there is even a comment talking about ssize_t … 3. We had a policy that we only export one single MPI level function per file in the mpi directory. You changed this as some of the files exports now two function (the original function together with the _x version). 4. In the OPAL datatype stuff sometimes you use size_t and sometimes ssize_t for the same type of logic (set and get count as an example). Why? 5. You change the comments in the opal_datatype.h with "question marks"? the cache boundary must be known, it can't be somewhere between x-y bytes ago … 6. I'm not sure the change of nbElems from uint32_t to size_t (in opal/datatype/opal_datatype.h) is doing what you expect… Btw, I have a question to you fellow MPI Forum attendees. I just can't remember why the MPI forum felt there was a need for the MPI_Type_get[_true]_extent_x? MPI_Count can't be bigger than MPI_Aint, so I don't see what is the benefit of extending the MPI_Type_get_true_extent(MPI_Datatype, MPI_Aint*, MPI_Aint*) and MPI_Type_get_extent(MPI_Datatype, MPI_Aint*, MPI_Aint*) with the corresponding _X versions? George. On Jul 16, 2013, at 21:14 , Nathan Hjelm <hje...@lanl.gov> wrote: > What: Add support for the MPI-3 MPI_Count datatype and functions: > MPI_Get_elements_x, MPI_Status_set_elements_x, MPI_Type_get_extent_x, > MPI_Type_get_true_extent_x, and MPI_Type_size_x. This will be CMR'd to 1.7.3 > if there are no objections. > > Why: MPI_Count is required by the MPI 3.0 standard. This will add another > checkmark by MPI 3 support. > > When: Setting a short timeout of one week (Tues, July 23, 2013). Most of the > changes add the new functionality but there are some changes that affect the > datatype engine. > > Details follow. > > -Nathan > > Repository @ github: https://github.com/hjelmn/ompi-count.git > > Relevant commits: > General support: > https://github.com/hjelmn/ompi-count/commit/db54d13404a241642fa783d5b3cc74edcb1103f2 > Fortran support: > https://github.com/hjelmn/ompi-count/commit/293adf84be52c2cd8acfe31be19cfe0afe14752d > Others: > https://github.com/hjelmn/ompi-count/commit/6c6ca8e539da675632c249c891ff93fdbc9d8de8 > > https://github.com/hjelmn/ompi-count/commit/9638ef1f245f12bb98abbf5f47e1ecfd1a018862 > > https://github.com/hjelmn/ompi-count/commit/e158aa152d122e554b89498f5a71284ce1361a99 > > Add support for MPI_Count type and MPI_COUNT datatype and add the required > MPI-3 functions MPI_Get_elements_x, MPI_Status_set_elements_x, > MPI_Type_get_extent_x, MPI_Type_get_true_extent_x, and MPI_Type_size_x. > This commit adds only the C bindings. Fortran bindins will be added in > another commit. For now the MPI_Count type is define to have the same size > as MPI_Offset. The type is required to be at least as large as MPI_Offset > and MPI_Aint. The type was initially intended to be a ssize_t (if it was > the same size as a long long) but there were issues compiling romio with > that definition (despite the inclusion of stddef.h). > > I updated the datatype engine to use size_t instead of uint32_t to support > large datatypes. This will require some review to make sure that 1) the > changes are beneficial, 2) nothing was broken by the change (I doubt > anything was), and 3) there are no performance regressions due to this > change. > > George, please look over these changes and let me know if you see anything > wrong with my updates to the datatype engine. > > -Nathan > _______________________________________________ > devel mailing list > de...@open-mpi.org > http://www.open-mpi.org/mailman/listinfo.cgi/devel
