On Tue, Jul 16, 2013 at 10:22:33PM +0200, George Bosilca wrote:
> 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.
Ah, yes. That is correct will fix that and update my repository now.
> 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 ?
Will fix this as well.
> 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).
I was trying to avoid having too much duplicate code. If including both functions in the same file is not ok I will move the _x functions to their own .c files.
> 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?
I replaced uint32_t with size_t and int32_t with ssize_t to be consistent with the original code.
> 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 ?
The problem is size_t can be either 4 or 8 bytes so there are two possible places for the cache boundary. If you prefer I can change those to use int64_t and uint64_t instead so we will know where the cache boundaries are. (or leave them as 32-bit if that is the correct answer).
> 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?
Admittedly, I changed the size of nbElems early on. I left it as 64-bit (32-bit on 32-bit platforms) to allow the creation of datatypes with more than 2^32 elements. Not sure this senario will ever occur though.
> 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?
It was before my involement with the forum. Jeff knows better why this was done.
Thanks for taking a look. I will let you know when I have fixed the ssize_t/size_t issue.