From 27fecda12cf51891f83b7d91d87fffd393d67e48 Mon Sep 17 00:00:00 2001 From: George Bosilca Date: Fri, 25 Feb 2011 20:43:17 +0000 Subject: [PATCH] Allow the one sided components to correctly retrieve the op to be applied. Correct the MPI validation process of the MPI_Accumulate arguments. Fix another potential problem not yet reported. If we convert the MPI datatypes direclty into OPAL datatypes, we will restrict their number to the locally different types. Which might not be identical on the remote node, if we are in a heterogeneous environment. So, for MPI One sided only deal with MPI level types, never simplify them on OPAL types (at least on the args). The unfortunate outcome is that we need to create the args for all datatypes. This commit was SVN r24466. --- ompi/datatype/ompi_datatype.h | 1 + ompi/datatype/ompi_datatype_args.c | 90 ++++++++++++++++++++++-- ompi/mca/osc/base/osc_base_obj_convert.c | 39 ++++------ ompi/mpi/c/accumulate.c | 58 ++------------- 4 files changed, 106 insertions(+), 82 deletions(-) diff --git a/ompi/datatype/ompi_datatype.h b/ompi/datatype/ompi_datatype.h index e3ee6b26bc..7d83b7400c 100644 --- a/ompi/datatype/ompi_datatype.h +++ b/ompi/datatype/ompi_datatype.h @@ -299,6 +299,7 @@ OMPI_DECLSPEC int32_t ompi_datatype_set_args( ompi_datatype_t* pData, OMPI_DECLSPEC int32_t ompi_datatype_copy_args( const ompi_datatype_t* source_data, ompi_datatype_t* dest_data ); OMPI_DECLSPEC int32_t ompi_datatype_release_args( ompi_datatype_t* pData ); +OMPI_DECLSPEC ompi_datatype_t* ompi_datatype_get_single_predefined_type_from_args( ompi_datatype_t* type ); /* * diff --git a/ompi/datatype/ompi_datatype_args.c b/ompi/datatype/ompi_datatype_args.c index 44610ff173..cbad9be49d 100644 --- a/ompi/datatype/ompi_datatype_args.c +++ b/ompi/datatype/ompi_datatype_args.c @@ -595,7 +595,7 @@ static ompi_datatype_t* __ompi_datatype_create_from_packed_description( void** p } array_of_datatype[i] = __ompi_datatype_create_from_packed_description( (void**)&next_buffer, - remote_processor ); + remote_processor ); if( NULL == array_of_datatype[i] ) { /* don't cleanup more than required. We can now modify these * values as we already know we have failed to rebuild the @@ -624,7 +624,7 @@ static ompi_datatype_t* __ompi_datatype_create_from_packed_description( void** p } #endif datatype = __ompi_datatype_create_from_args( array_of_length, array_of_disp, - array_of_datatype, create_type ); + array_of_datatype, create_type ); *packed_buffer = next_buffer; cleanup_and_exit: for( i = 0; i < number_of_datatype; i++ ) { @@ -636,7 +636,6 @@ static ompi_datatype_t* __ompi_datatype_create_from_packed_description( void** p return datatype; } - static ompi_datatype_t* __ompi_datatype_create_from_args( int32_t* i, MPI_Aint* a, ompi_datatype_t** d, int32_t type ) { @@ -645,37 +644,64 @@ static ompi_datatype_t* __ompi_datatype_create_from_args( int32_t* i, MPI_Aint* switch(type){ /******************************************************************/ case MPI_COMBINER_DUP: + /* should we duplicate d[0]? */ + /* ompi_datatype_set_args( datatype, 0, NULL, 0, NULL, 1, d[0], MPI_COMBINER_DUP ); */ break; /******************************************************************/ case MPI_COMBINER_CONTIGUOUS: ompi_datatype_create_contiguous( i[0], d[0], &datatype ); + ompi_datatype_set_args( datatype, 1, &i, 0, NULL, 1, d, MPI_COMBINER_CONTIGUOUS ); break; /******************************************************************/ case MPI_COMBINER_VECTOR: ompi_datatype_create_vector( i[0], i[1], i[2], d[0], &datatype ); + { + int* a_i[3]; a_i[0] = &i[0]; a_i[1] = &i[1]; a_i[2] = &i[2]; + ompi_datatype_set_args( datatype, 3, a_i, 0, NULL, 1, d, MPI_COMBINER_VECTOR ); + } break; /******************************************************************/ case MPI_COMBINER_HVECTOR_INTEGER: case MPI_COMBINER_HVECTOR: ompi_datatype_create_hvector( i[0], i[1], a[0], d[0], &datatype ); + { + int* a_i[2]; a_i[0] = &i[0]; a_i[1] = &i[1]; + ompi_datatype_set_args( datatype, 2, a_i, 1, a, 1, d, MPI_COMBINER_HVECTOR ); + } break; /******************************************************************/ case MPI_COMBINER_INDEXED: /* TO CHECK */ ompi_datatype_create_indexed( i[0], &(i[1]), &(i[1+i[0]]), d[0], &datatype ); + { + int* a_i[3]; a_i[0] = &i[0]; a_i[1] = &i[1]; a_i[2] = &(i[1+i[0]]); + ompi_datatype_set_args( datatype, 2 * i[0] + 1, a_i, 0, NULL, 1, d, MPI_COMBINER_INDEXED ); + } break; /******************************************************************/ case MPI_COMBINER_HINDEXED_INTEGER: case MPI_COMBINER_HINDEXED: ompi_datatype_create_hindexed( i[0], &(i[1]), a, d[0], &datatype ); + { + int* a_i[2]; a_i[0] = &i[0]; a_i[1] = &i[1]; + ompi_datatype_set_args( datatype, i[0] + 1, a_i, i[0], a, 1, d, MPI_COMBINER_HINDEXED ); + } break; /******************************************************************/ case MPI_COMBINER_INDEXED_BLOCK: ompi_datatype_create_indexed_block( i[0], i[1], &(i[2]), d[0], &datatype ); + { + int* a_i[3]; a_i[0] = &i[0]; a_i[1] = &i[1]; a_i[2] = &i[2]; + ompi_datatype_set_args( datatype, 2 * i[0], a_i, 0, NULL, 1, d, MPI_COMBINER_INDEXED_BLOCK ); + } break; /******************************************************************/ case MPI_COMBINER_STRUCT_INTEGER: case MPI_COMBINER_STRUCT: ompi_datatype_create_struct( i[0], &(i[1]), a, d, &datatype ); + { + int* a_i[2]; a_i[0] = &i[0]; a_i[1] = &i[1]; + ompi_datatype_set_args( datatype, i[0] + 1, a_i, i[0], a, i[0], d, MPI_COMBINER_STRUCT ); + } break; /******************************************************************/ case MPI_COMBINER_SUBARRAY: @@ -689,6 +715,12 @@ static ompi_datatype_t* __ompi_datatype_create_from_args( int32_t* i, MPI_Aint* pos += pArgs->i[0]; pArgs->i[pos] = i[4][0]; */ +#if 0 + { + int* a_i[5]; a_i[0] = &i[0]; a_i[1] = &i[1 + 0 * i[0]]; a_i[2] = &i[1 + 1 * i[0]]; a_i[3] = &i[1 + 2 * i[0]]; + ompi_datatype_set_args( datatype, 3 * i[0] + 2, a_i, 0, NULL, 1, d, MPI_COMBINER_SUBARRAY); + } +#endif break; /******************************************************************/ case MPI_COMBINER_DARRAY: @@ -707,6 +739,14 @@ static ompi_datatype_t* __ompi_datatype_create_from_args( int32_t* i, MPI_Aint* pos += i[2][0]; pArgs->i[pos] = i[7][0]; */ +#if 0 + { + int* a_i[8]; a_i[0] = &i[0]; a_i[1] = &i[1]; a_i[2] = &i[2]; + a_i[3] = &i[1 + 0 * i[0]]; a_i[4] = &i[1 + 1 * i[0]]; a_i[5] = &i[1 + 2 * i[0]]; + a_i[6] = &i[1 + 3 * i[0]]; a_i[7] = &i[1 + 4 * i[0]]; + ompi_datatype_set_args( datatype, 4 * i[0] + 4,a_i, 0, NULL, 1, d, MPI_COMBINER_DARRAY); + } +#endif break; /******************************************************************/ case MPI_COMBINER_F90_REAL: @@ -721,6 +761,7 @@ static ompi_datatype_t* __ompi_datatype_create_from_args( int32_t* i, MPI_Aint* break; /******************************************************************/ case MPI_COMBINER_RESIZED: + /*ompi_datatype_set_args( datatype, 0, NULL, 2, a, 1, d, MPI_COMBINER_RESIZED );*/ break; /******************************************************************/ default: @@ -730,14 +771,13 @@ static ompi_datatype_t* __ompi_datatype_create_from_args( int32_t* i, MPI_Aint* return datatype; } - ompi_datatype_t* ompi_datatype_create_from_packed_description( void** packed_buffer, struct ompi_proc_t* remote_processor ) { ompi_datatype_t* datatype; datatype = __ompi_datatype_create_from_packed_description( packed_buffer, - remote_processor ); + remote_processor ); /* Keep the pointer aligned to MPI_Aint */ OMPI_DATATYPE_ALIGN_PTR(*packed_buffer, void*); if( NULL == datatype ) { @@ -746,3 +786,43 @@ ompi_datatype_t* ompi_datatype_create_from_packed_description( void** packed_buf ompi_datatype_commit( &datatype ); return datatype; } + +/** + * Parse the datatype description from the args and find if the + * datatype is created from a single predefined type. If yes, + * return the type, otherwise return NULL. + */ +ompi_datatype_t* ompi_datatype_get_single_predefined_type_from_args( ompi_datatype_t* type ) +{ + ompi_datatype_t *predef = NULL, *current_type, *current_predef; + ompi_datatype_args_t* args = (ompi_datatype_args_t*)type->args; + int i; + + if( ompi_datatype_is_predefined(type) ) + return type; + + for( i = 0; i < args->cd; i++ ) { + current_type = args->d[i]; + if( ompi_datatype_is_predefined(current_type) ) { + current_predef = current_type; + } else { + current_predef = ompi_datatype_get_single_predefined_type_from_args(current_type); + if( NULL == current_predef ) { /* No single predefined datatype */ + return NULL; + } + } + if( NULL == predef ) { /* This is the first iteration */ + predef = current_predef; + } else { + /** + * What exactly should we consider as identical types? If they are + * the same MPI level type, or if they map to the same OPAL datatype? + * In other words, MPI_FLOAT and MPI_REAL4 are they identical? + */ + if( predef != current_predef ) { + return NULL; + } + } + } + return predef; +} diff --git a/ompi/mca/osc/base/osc_base_obj_convert.c b/ompi/mca/osc/base/osc_base_obj_convert.c index 9531f45c2f..09fa9fe68d 100644 --- a/ompi/mca/osc/base/osc_base_obj_convert.c +++ b/ompi/mca/osc/base/osc_base_obj_convert.c @@ -39,30 +39,24 @@ ompi_osc_base_get_primitive_type_info(ompi_datatype_t *datatype, ompi_datatype_t **prim_datatype, uint32_t *prim_count) { - struct ompi_datatype_t *primitive_datatype = NULL; - uint32_t primitive_count; + ompi_datatype_t *primitive_datatype = NULL; + size_t datatype_size, primitive_size, primitive_count; - /* get underlying type... */ - if (ompi_datatype_is_predefined(datatype)) { - primitive_datatype = datatype; - primitive_count = 1; - } else { - int i, found_index = -1; - uint32_t mask = 1; - for (i = 0 ; i < OMPI_DATATYPE_MAX_PREDEFINED ; ++i) { - if (datatype->super.bdt_used & mask) { - found_index = i; - break; - } - mask *= 2; - } - primitive_datatype = (ompi_datatype_t*) - ompi_datatype_basicDatatypes[found_index]; - primitive_count = datatype->super.nbElems; + primitive_datatype = ompi_datatype_get_single_predefined_type_from_args(datatype); + if( NULL == primitive_datatype ) { + *prim_count = 0; + return OMPI_SUCCESS; } + ompi_datatype_type_size( datatype, &datatype_size ); + ompi_datatype_type_size( primitive_datatype, &primitive_size ); + primitive_count = datatype_size / primitive_size; +#if OPAL_ENABLE_DEBUG + assert( 0 == (datatype_size % primitive_size) ); +#endif /* OPAL_ENABLE_DEBUG */ + /* We now have the count as a size_t, convert it to an uint32_t */ *prim_datatype = primitive_datatype; - *prim_count = primitive_count; + *prim_count = (uint32_t)primitive_count; return OMPI_SUCCESS; } @@ -198,16 +192,13 @@ ompi_osc_base_process_op(void *outbuf, ompi_op_reduce(op, inbuf, outbuf, count, datatype); } else { struct ompi_datatype_t *primitive_datatype = NULL; - uint32_t primitive_count; ompi_osc_base_convertor_t convertor; struct iovec iov; uint32_t iov_count = 1; size_t max_data; struct opal_convertor_master_t master = {NULL, 0, 0, 0, {0, }, NULL}; - ompi_osc_base_get_primitive_type_info( datatype, - &primitive_datatype, - &primitive_count ); + primitive_datatype = ompi_datatype_get_single_predefined_type_from_args(datatype); /* create convertor */ OBJ_CONSTRUCT(&convertor, ompi_osc_base_convertor_t); diff --git a/ompi/mpi/c/accumulate.c b/ompi/mpi/c/accumulate.c index 2e1e298c8a..74bc0c4da1 100644 --- a/ompi/mpi/c/accumulate.c +++ b/ompi/mpi/c/accumulate.c @@ -81,7 +81,7 @@ int MPI_Accumulate(void *origin_addr, int origin_count, MPI_Datatype origin_data /* While technically the standard probably requires that the datatypes used with MPI_REPLACE conform to all the rules for other reduction operators, we don't require such - behaivor, as checking for it is expensive here and we don't + behavior, as checking for it is expensive here and we don't care in implementation.. */ if (op != &ompi_mpi_op_replace.op) { ompi_datatype_t *op_check_dt, *origin_check_dt; @@ -90,65 +90,17 @@ int MPI_Accumulate(void *origin_addr, int origin_count, MPI_Datatype origin_data /* ACCUMULATE, unlike REDUCE, can use with derived datatypes with predefinied operations, with some restrictions outlined in MPI-2:6.3.4. The derived - datatype must be composed entirley from one predefined + datatype must be composed entierly from one predefined datatype (so you can do all the construction you want, but at the bottom, you can only use one datatype, say, MPI_INT). If the datatype at the target isn't predefined, then make sure it's composed of only one datatype, and check that datatype against ompi_op_is_valid(). */ - if (opal_datatype_is_predefined(&(origin_datatype->super))) { - origin_check_dt = origin_datatype; - } else { - int i, found_index = -1, num_found = 0; - uint32_t mask = 1; + origin_check_dt = ompi_datatype_get_single_predefined_type_from_args(origin_datatype); + op_check_dt = ompi_datatype_get_single_predefined_type_from_args(target_datatype); - for (i = 0 ; i < OMPI_DATATYPE_MAX_PREDEFINED ; ++i) { - if (origin_datatype->super.bdt_used & mask) { - num_found++; - found_index = i; - } - mask *= 2; - } - if (found_index < 0 || num_found > 1) { - /* this is an erroneous datatype. Let - ompi_op_is_valid tell the user that */ - OMPI_ERRHANDLER_RETURN(MPI_ERR_TYPE, win, MPI_ERR_TYPE, FUNC_NAME); - } else { - origin_check_dt = (ompi_datatype_t*) - ompi_datatype_basicDatatypes[found_index]; - } - } - - if (opal_datatype_is_predefined(&(target_datatype->super))) { - op_check_dt = target_datatype; - } else { - int i, found_index = -1, num_found = 0; - uint32_t mask = 1; - - for (i = 0 ; i < OMPI_DATATYPE_MAX_PREDEFINED ; ++i) { - if (target_datatype->super.bdt_used & mask) { - num_found++; - found_index = i; - } - mask *= 2; - } - if (found_index < 0 || num_found > 1) { - /* this is an erroneous datatype. Let - ompi_op_is_valid tell the user that */ - OMPI_ERRHANDLER_RETURN(MPI_ERR_TYPE, win, MPI_ERR_TYPE, FUNC_NAME); - } else { - /* datatype passes muster as far as restrictions - in MPI-2:6.3.4. Is the primitive ok with the - op? Unfortunately have to cast away - constness... */ - op_check_dt = (ompi_datatype_t*) - ompi_datatype_basicDatatypes[found_index]; - } - } - - /* check to make sure same primitive type */ - if (op_check_dt != origin_check_dt) { + if( !((origin_check_dt == op_check_dt) & (NULL != op_check_dt)) ) { OMPI_ERRHANDLER_RETURN(MPI_ERR_ARG, win, MPI_ERR_ARG, FUNC_NAME); }