MPI_Accumulate Error when using datatype "MPI_SHORT_INT"

[Zhiming-Wang]

Hi, all

There is a MPI_Accumulate error at Open MPI when using datatype "MPI_SHORT_INT".

I thought that it was about the compress/uncompress of predefined datatype "MPI_SHORT_INT" at osc module.

The datatype "MPI_SHORT_INT" used at this case, its size of storage space is 8 byte (4-byte aligned), but its payload size is 6 byte (2 byte for short, and 4 byte for int.). When updating data from remote ranks, it was compressed (6 byte for each), but uncompress was not done when reading data, 8 byte was get for each element, so generated an error.
Please confirm and help to fix it. Thanks very much.

Source code of my test case:

#include <stdio.h>
#include "mpi.h"

#define LEN 100
#define  SHIFT -1

int main( int argc, char **argv ) {

    int my_rank, num_tasks;
    int i, j;
    MPI_Win win;
    MPI_Group all_group, origin_group, target_group;
    int root = 0;

    typedef struct {
        short val;
        short pad;
        int rank;
    } short_int_t;

    short_int_t Win[LEN], Array[LEN];

    MPI_Init( &argc, &argv );
    MPI_Comm_rank( MPI_COMM_WORLD, &my_rank );
    MPI_Comm_size( MPI_COMM_WORLD, &num_tasks );
    if (2 != num_tasks) {
        printf("There should be 2 ranks at this case. \n");
        MPI_Abort(MPI_COMM_WORLD, 1);
    }



    MPI_Win_create( Win, sizeof(Win), sizeof(short_int_t),
                    MPI_INFO_NULL, MPI_COMM_WORLD, &win );

    MPI_Win_fence( 0, win );
    MPI_Comm_group( MPI_COMM_WORLD, &all_group );
    MPI_Group_incl( all_group, 1, &root, &target_group );
    MPI_Group_difference( all_group, target_group, &origin_group );

    /* initialize all arrays for MPI_MINLOC*/
    for( i=0; i<LEN; i++ ) {
        if (my_rank == i%2) {
            Array[i].val = Win[i].val = SHIFT;
        }
        else {
            Array[i].val = Win[i].val = -1*SHIFT;
        }
        Array[i].rank = my_rank;
        Win[i].rank = my_rank;
        Win[i].pad = -i;
    }

    if( my_rank == root ) {

        MPI_Win_post( origin_group, 0, win );

        MPI_Win_wait( win );
    }
    else {
        MPI_Win_start( target_group, 0, win );

        MPI_Accumulate( Array, LEN, MPI_SHORT_INT, root, 0,
                        LEN, MPI_SHORT_INT, MPI_MINLOC, win );

        MPI_Win_complete( win );
    }

    MPI_Win_fence( 0, win );

    /* check results */
    if( my_rank == root ) {

        for( i=0; i<LEN; i++ ) {
            if( Win[i].val != SHIFT ) {
                printf( "MPI_Accumulate failed to obtain the proper value. The %dth should be %d, but it is %d\n", i, SHIFT, Win[i].val);
                MPI_Abort( MPI_COMM_WORLD, -1 );
            }

            if( Win[i].rank != i%2) {
                printf("MPI_Accumulate failed to obtain the proper rank. \n");
                MPI_Abort( MPI_COMM_WORLD, -1 );
            }

            if( Win[i].pad != -i ) {
                printf("MPI_Accumulate clobbered the pad.\n");
                MPI_Abort( MPI_COMM_WORLD, -1 );
            }
        }
        printf("MPI_Accumulate Check PASSED. \n");
    }

    MPI_Win_free( &win );

    MPI_Finalize();

    return(0);
}

[Zhiming-Wang]

I have fixed the TCP branch. It is ugly, but it works at my test case.
I added a temp buffer for converting 6 byte to 8 byte using "osc_pt2pt_copy_on_recv".

diff --git a/ompi/mca/osc/pt2pt/osc_pt2pt_data_move.c b/ompi/mca/osc/pt2pt/osc_pt2pt_data_move.c
index 347ea3c..d763da8 100644
--- a/ompi/mca/osc/pt2pt/osc_pt2pt_data_move.c
+++ b/ompi/mca/osc/pt2pt/osc_pt2pt_data_move.c
@@ -595,10 +595,43 @@ static inline int osc_pt2pt_accumulate_buffer (void *target, void *source, size_
         free(buffer);
     } else
#endif
+    {
+        if (ompi_datatype_is_contiguous_memory_layout(datatype, count)) {
+            /* copy the data from the temporary buffer into the user window */
+            ret = ompi_osc_base_process_op(target, source, source_len, datatype,
+                                      count, op);
+        }
+        else {
+            size_t buflen;
+            char *buffer;
+            char *buffer_process_op;
+            ptrdiff_t true_lb, true_extent, lb, extent;
+
+            /* extent is from lb to ub (e.g., MPI_SHORT_INT is 8) */
+            ompi_datatype_get_extent(datatype, &lb, &extent);
+            /* true_extent is extent of actual type map, ignoring lb and ub
+               (e.g., MPI_SHORT_INT is 8) */
+            ompi_datatype_get_true_extent(datatype, &true_lb, &true_extent);
+
+            buflen = true_extent + (count - 1) * extent;
+            buffer = (void *)malloc (buflen);
+            if (OPAL_UNLIKELY(NULL == buffer)) {
+                return OMPI_ERR_OUT_OF_RESOURCE;
+            }

-    /* copy the data from the temporary buffer into the user window */
-    ret = ompi_osc_base_process_op(target, source, source_len, datatype,
-                                   count, op);
+            bzero(buffer, buflen);
+            buffer_process_op = buffer - true_lb;
+            osc_pt2pt_copy_on_recv (buffer_process_op, source, source_len, proc,
+                                        count, datatype);
+
+            ret = ompi_osc_base_process_op(target, buffer_process_op, source_len,
+                                        datatype, count, op);
+
+            free(buffer);
+        }
+    }

     return ret;
}

There should be a similar process at "rdma" osc component for OpenIB. But I haven't found her yet.
I'm not sure that all branches can be covered by my modification. I'm also not sure whether the compress and uncompress mechanism at osc for MPI_SHORT_INT continue to work at one-sided communications. How about MPI_DOUBLE_INT and MPI_LONG_INT?

[nysal]

@hjelmn Can you take a look at this one?

[hjelmn]

MPI_SHORT_INT is not a valid datatype for accumulate operations. From MPI 3-1 § 11.3.4 pp 425:2-8:

Each datatype argument must be a predefined datatype or a derived datatype, where
all basic components are of the same predefined datatype. Both datatype arguments must
be constructed from the same predefined datatype. The operation op applies to elements of
that predefined type. The parameter target_datatype must not specify overlapping entries,
and the target buffer must fit in the target window.

Look at the table in Annex A.1 pp 673. MPI_SHORT_INT is not a "predefined type" and is not made up entirely of a single predefined type.

[nysal]

mpich does have a test that uses MPI_2INT - http://git.mpich.org/mpich.git/blob/HEAD:/test/mpi/rma/acc-loc.c
MPI_SHORT_INT belongs to the same group of predefined datatypes for reduction functions (Annex A.1 page 657). The "where
all basic components are of the same predefined datatype" could be interpreted as a remark about derived dataypes.
The http://git.mpich.org/mpich.git/blob/HEAD:/test/mpi/rma/acc-pairtype.c test uses a vector of MPI_LONG_DOUBLE_INT

[hjelmn]

MPI_2INT is definitely ok as it is made up of 2 of the same predefined datatypes. I will have top ask the forum wg on the other one. MPICH may be violating the standard here with the MPI_LONG_DOUBLE_INT usage. My understanding is only those on the table i referenced or derived datatypes made up of a single one from the table are allowed with accumulate.

[nysal]

ok, In that case I'll reopen the issue and then we can take appropriate action once we get a clarification from the RMA wg.

[hjelmn]

Forum agrees with you. That said, I have a fundamental disagreement with the standard over the existence of MPI_MAXLOC/MPI_MINLOC. They require special types that do not even begin to cover the types users might want to use (there is an open forum ticket to add more). I am going to push for deprecation in MPI-4. In the meantime for compliance I will add support for these non-contiguous (which is the real problem) type. The fix on this ticket is incorrect. I need to talk to more people on the forum to figure out the optimal way forward.

[Zhiming-Wang]

@hjelmn Thanks very much.

[nysal]

@hjelmn Did you get a chance to look at this one?

[hjelmn]

Not yet. The fix is actually really complicated. Still planning on pushing on deprecation but need to do some searching to see where these ops are used.

To be clear this is a very low priority ticket for me. I don't think anyone should use this functionality and I will enable it for completeness in the 2.1 or 2.2 timeframe.