Always ensure consistency of new MPI datatypes

[giordano]

Also:

• skip test of custom primitive type of size 80 on 32-bit systems in Julia v1.12+, as the MPI extent doesn't match Julia's aligned size
• add a test for a new primitive datatype larger than 64 bits which should work on 32-bit systems

With this PR, on a 32-bit linux I get

julia> using MPI

julia> MPI.Init();

julia> primitive type Primitive80 80 end

julia> MPI.Datatype(Primitive80)
ERROR: AssertionError: The MPI extent of type Primitive80 (12) does not match the size expected by Julia (16)
Stacktrace:
 [1] MPI.Datatype(::Type{Primitive80})
   @ MPI ~/.julia/packages/MPI/FiDGt/src/datatypes.jl:171
 [2] top-level scope
   @ REPL[4]:1

which should be better than silently getting incorrect results at runtime.

Fix #853.

@simonbyrne @vchuravy can you see cases where the consistency test added inside the constructor of the MPI.Datatype can cause problems?

[giordano]

Note: I'm adding this check after the type is registered (we need that to be able to call MPI.Types.extent) and added to the list of created datatypes, which means that the type is always added there even if the consistency check fails. I'm wondering if this should be moved after the init() call above, but I don't know what happens to the created_datatypes dictionary if the test errors out. So for safety I kept it outside of the get!.

[vchuravy]

LGTM, but we are the ones who tells MPI about size and alignment, are we not?

MPI.jl/src/datatypes.jl

Line 433 in 0d57c63

function create!(newtype::Datatype, ::Type{T}) where {T}

[giordano]

Uhm, yes and no, in the sense that we create a datatype of size sizeof(T):

MPI.jl/src/datatypes.jl

Line 441 in 0d57c63

szrem = sz = sizeof(T)

and for Primitive80 we have

blocklengths = [1, 1]
displacements = [0, 8]
types = MPI.Datatype[MPI.Datatype(UInt64): MPI_UINT64_T, MPI.Datatype(UInt16): MPI_UINT16_T]

but MPI.Types.extent(MPI.Datatype(Primitive80))[2] is 12 on i686, not 10, I don't quite know where that number comes from (I'm assuming MPI does internally some sort of alignment?).

I don't know if it makes sense, but one option could be to create an MPI datype of size

diff --git a/src/datatypes.jl b/src/datatypes.jl
index 9dd721fc..f46c0331 100644
--- a/src/datatypes.jl
+++ b/src/datatypes.jl
@@ -438,7 +438,8 @@ function create!(newtype::Datatype, ::Type{T}) where {T}
 
     if isprimitivetype(T)
         # primitive type
-        szrem = sz = sizeof(T)
+        al = Base.datatype_alignment(T)
+        szrem = sz = cld(sizeof(T), al) * al
         disp = 0
         for (i,basetype) in (8 => Datatype(UInt64), 4 => Datatype(UInt32), 2 => Datatype(UInt16), 1 => Datatype(UInt8))
             if sz == i

i.e. the same check introduced here, but that still doesn't necessarily ensure MPI.Types.extent returns the same value, although it's probably more likely to do it, and I can confirm that with this change all test_datatype.jl tests will pass on i686, including for Primitive80 (yay!). I'm assuming what matters to MPI is the memory allocation size of the object (they're elements of arrays, not individual instances of the types), rather than the strict size?

What do you think?

[giordano]

but that still doesn't necessarily ensure MPI.Types.extent returns the same value

Ok, I went down a rabbit hole reading the source code of OpenMPI (much more readable than MPICH) and I found the following:

• MPI_Types_get_extent simply returns the extent set during the construction of the datatype, it does no magic on its own
• MPI_Types_create_struct does some arithmetic internally (I haven't read it very carefully though)

OpenMPI documentation for MPI_Type_create_struct has the following note:

If an upper bound is set explicitly by using the MPI datatype MPI_UB, the corresponding index must be positive.

The MPI-1 Standard originally made vague statements about padding and alignment; this was intended to allow the simple definition of structures that could be sent with a count greater than one. For example,

struct {int a; char b;} foo;

may have

sizeof(foo) = sizeof(int) + sizeof(char);

defining the extent of a datatype as including an epsilon, which would have allowed an implementation to make the extent an MPI datatype for this structure equal to 2*sizeof(int). However, since different systems might define different paddings, a clarification to the standard made epsilon zero. Thus, if you define a structure datatype and wish to send or receive multiple items, you should explicitly include an MPI_UB entry as the last member of the structure. See the above example.

However MPI_UB is deprecated

[vchuravy]

Clear as mud https://docs.open-mpi.org/en/v5.0.5/building-apps/removed-mpi-constructs.html#stop-using-mpi-lb-mpi-ub

[giordano]

Clear as mud docs.open-mpi.org/en/v5.0.5/building-apps/removed-mpi-constructs.html#stop-using-mpi-lb-mpi-ub

Yeah, I mentioned MPI_UB is actually deprecated 🙂

I pushed a commit to change the size of the datatype from simply sizeof(T) to cld(Base.datatype_alignment(T), sizeof(T)) * Base.datatype_alignment(T): when Base.datatype_alignment(T) == sizeof(T) there's no change compared to now, in the other cases we should have more consistent sizes. I added also a test with a primitive type larger than 128 bits, it's passing for me locally on i686.

[giordano]

I did some quick research to see if there was any prior art about this:

• for rsmpi in the UserDatatype struct I don't see automatic constructors from custom Rust datatypes
• likewise, for mpi4py I'm not see automatic features to create an MPI datatype from a custom python type (if you can even create it?)

Maybe I'm missing something (totally possible!) but I suspect we're on our own here.