Skip to content

Conversation

@devreal
Copy link
Contributor

@devreal devreal commented Oct 23, 2025

Utilize the count and disp arrays for type-punning of int and MPI_Count arguments to datatype creation functions. Store and pack integer or size_t depending on what is needed. Adjust places where bigcount support for datatypes was missing.

This is work in progress and not yet properly tested, other than compiling through it. I wanted to put it out to get some eyes on it to see whether people agree with the design and that it's the right direction, before putting more effort into testing.

@devreal devreal requested review from Copilot and hppritcha October 23, 2025 00:51
@devreal devreal marked this pull request as draft October 23, 2025 00:51
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR implements bigcount support for MPI datatypes by introducing type-punned count and displacement arrays that can store either 32-bit integers or 64-bit size_t/ptrdiff_t values. The implementation uses the least significant bit of pointers as a flag to distinguish between the two representations, allowing the datatype framework to handle both standard MPI and bigcount MPI_Count variants transparently.

Key changes:

  • Introduced opal_count_array_t and opal_disp_array_t wrapper types with helper functions for type-punned storage
  • Updated datatype creation functions to accept these wrapper types instead of raw int/ptrdiff_t arrays
  • Modified datatype argument storage to track both 32-bit and 64-bit count arrays separately
  • Removed conditional bigcount compilation blocks and temporary array allocations throughout the codebase

Reviewed Changes

Copilot reviewed 54 out of 54 changed files in this pull request and generated 9 comments.

Show a summary per file
File Description
opal/util/count_disp_array.h New header defining type-punned array wrapper types and manipulation functions
opal/util/Makefile.am Added new header to build system
ompi/util/count_disp_array.h Refactored to delegate to opal implementation
ompi/datatype/ompi_datatype.h Updated function signatures to use count_array_t types and size_t for counts
ompi/datatype/ompi_datatype_args.c Rewrote argument packing/unpacking to handle both int and size_t arrays
ompi/datatype/ompi_datatype_create*.c Updated to use count_array_t wrappers and size_t
ompi/mpi/c/type_*.c.in Removed bigcount overflow checks and temp arrays, use wrapper types
ompi/mca/*/**.c Updated datatype creation calls to use wrapper macros
opal/datatype/opal_datatype_*.c Changed parameter types from int32_t to ssize_t/size_t

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@devreal devreal requested a review from bosilca October 23, 2025 15:28
@hppritcha
Copy link
Member

okay this is pretty big! i will probably not be able to get to most of this till Monday.

@devreal devreal force-pushed the bigcount-datatypes branch from 306687d to 5f3af4e Compare October 25, 2025 17:16
@devreal
Copy link
Contributor Author

devreal commented Oct 27, 2025

CI currently fails for NVIDIA because hcoll was not adapted but I figure I'll just wait for #13467 to land. Also, I plan to port the datatype tests for bigcount, ideally without having to duplicate the whole code.

Copy link
Member

@hppritcha hppritcha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why did you remove this?
I'll just need to put it back in for the ABI support.

@hppritcha
Copy link
Member

why did you remove this? I'll just need to put it back in for the ABI support.

type_get_envelop.c.in?

@devreal
Copy link
Contributor Author

devreal commented Oct 27, 2025

why did you remove this? I'll just need to put it back in for the ABI support.

type_get_envelop.c.in?

It didn't seem like it was being compiled (it had broken code and no compiled complained). The bigcount version has an additional parameter (number of counts) so it cannot just be generated.

@hppritcha
Copy link
Member

ah yes. i forgot in the ABI work i enhanced the python to handle the case of no-big-count vs big-count and could use the template. so no worries for now.

@janjust
Copy link
Contributor

janjust commented Oct 28, 2025

CI currently fails for NVIDIA because hcoll was not adapted but I figure I'll just wait for #13467 to land. Also, I plan to port the datatype tests for bigcount, ideally without having to duplicate the whole code.

@devreal done, PR merged

Utilize the count and disp arrays for type-punning of int and MPI_Count
arguments to datatype creation functions. Pack integer or MPI_Count
depending on what is needed when packing a datatype. Adjust places
where bigcount support for datatpyes was missing.

Signed-off-by: Joseph Schuchart <[email protected]>
The signatures for MPI_Type_get_envelope and MPI_Type_get_envelope_c are
different so we cannot generate them. This file seems to be unused.

Signed-off-by: Joseph Schuchart <[email protected]>
Signed-off-by: Joseph Schuchart <[email protected]>
@devreal devreal force-pushed the bigcount-datatypes branch from f03693e to 9090b44 Compare October 29, 2025 02:04
For struct, hindex, and hindex_block MPI_Aint arguments turn into
MPI_Count arguments for bigcount. This needs special handling.

Signed-off-by: Joseph Schuchart <[email protected]>
This was a fragment of a previous change that got only partially rolled back.

Signed-off-by: Joseph Schuchart <[email protected]>
That codepath wasn't compiled so it needed some adjustment.

Signed-off-by: Joseph Schuchart <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants