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

Big Count: first pass for coll framework bc #12539

Merged
merged 1 commit into from
May 20, 2024

Conversation

hppritcha
Copy link
Member

This commit adds only those functions which make use of C integer promotion. So none of the 'v,w' and reduce_scatter related methods are added in this PR.

Related to #12336
Pieces of #12478 were taken out to make this PR.

Copy link
Contributor

@devreal devreal left a comment

Choose a reason for hiding this comment

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

Some comments: I prefer %zu over %lu for size_t (I stopped marking them at some point) and there is a place that can be simplified with the size_t conversion.

ompi/mca/coll/adapt/coll_adapt_ibcast.c Outdated Show resolved Hide resolved
ompi/mca/coll/adapt/coll_adapt_ireduce.c Outdated Show resolved Hide resolved
ompi/mca/coll/adapt/coll_adapt_ireduce.c Outdated Show resolved Hide resolved
ompi/mca/coll/base/coll_base_allreduce.c Outdated Show resolved Hide resolved
ompi/mca/coll/base/coll_base_allreduce.c Outdated Show resolved Hide resolved
ompi/mca/coll/base/coll_base_allreduce.c Outdated Show resolved Hide resolved
ompi/mca/coll/base/coll_base_allreduce.c Show resolved Hide resolved
ompi/mca/coll/base/coll_base_allreduce.c Outdated Show resolved Hide resolved
ompi/mca/coll/base/coll_base_allreduce.c Show resolved Hide resolved
ompi/mca/coll/base/coll_base_scatter.c Show resolved Hide resolved
@jsquyres
Copy link
Member

jsquyres commented May 9, 2024

Some comments: I prefer %zu over %lu for size_t (I stopped marking them at some point) and there is a place that can be simplified with the size_t conversion.

Is there a reason to not use PRIsize_t? I thought that was guaranteed to be the Right Thing.

https://github.com/open-mpi/ompi/wiki/PrintfCodes

@devreal
Copy link
Contributor

devreal commented May 9, 2024

%zu is the format specifier for size_t, since C99 :) much more readable than the macro string concatenation https://en.cppreference.com/w/c/io/fprintf

Copy link
Contributor

@devreal devreal left a comment

Choose a reason for hiding this comment

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

Thanks @hppritcha! My picky clang didn't find any warnings related to these changes.

@hppritcha hppritcha requested review from bosilca and removed request for bosilca May 14, 2024 15:37
This commit adds only those functions which make use of C integer promotion.
So none of the 'v,w' and reduce_scatter related methods are added in this PR.

Related to open-mpi#12336
Pieces of open-mpi#12478 were taken out to make this PR.

Signed-off-by: Howard Pritchard <[email protected]>
@hppritcha hppritcha force-pushed the coll-framework-bigcount-intpromo branch from 118a843 to 4d7c65d Compare May 20, 2024 15:42
@hppritcha hppritcha merged commit 6e99e02 into open-mpi:main May 20, 2024
14 checks passed
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.

4 participants