Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ompi/datatype: use size_t for count arguments #12351

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

wenduwan
Copy link
Contributor

The use of int for count arguments is becoming restrictive especially to adopt large count. This change extends the argument type to size_t.

The use of int for count arguments is becoming restrictive especially to
adopt large count. This change extends the argument type to size_t.

Signed-off-by: Wenduo Wang <wenduwan@amazon.com>
@bosilca
Copy link
Member

bosilca commented Feb 20, 2024

This change in API will break the MPI datatype management in many ways (basically everything that relies on the datatype storage description such as one-sided, IO, and all the datatype representation manipulation functions, the combiner_*). The root cause is that the internal storage format for the datatype, and all the functions to expose it to other libraries are based on int32_t.

I have the feeling that the correct solution is to split the datatype API in two, one used by MPI where we follow MPI expectations, and the internal one where we can use counts but we will not provide any data representation support (the support will remain at the MPI level). Based on this, we will only use the internal API inside OMPI and the external API will be reserved for the MPI layer.

@wenduwan
Copy link
Contributor Author

I also had the concern of breaking API compatibility but none of our CI(internal and gh action) actually caught this so that's interesting.

I was reading the code and saw that the count argument is sometimes typed size_t like here - so I have a hunch that it should work somehow and it is possible to change the type safely.

I also agree with the proposed solution to introduce another internal API - I haven't yet figured out how.

@jsquyres
Copy link
Member

jsquyres commented Mar 5, 2024

@wenduwan It looks like you are still working on this. Do you want to move this PR to Draft?

@wenduwan wenduwan marked this pull request as draft March 5, 2024 16:16
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.

None yet

3 participants