-
Notifications
You must be signed in to change notification settings - Fork 302
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
Add new style allgather collective #137
Conversation
This is the first "new style" collective that doesn't use the pattern where it is initialized once, holds some state, and is used many times. The new style is intended for use where collectives can be called at any time, such that initialization can no longer be amortized because reuse may no longer apply.
Like gather in the previous commit, this is a single function and doesn't hold on to any state. This implements a ring allgather.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, will approve when it's rebased and imported
// Sanity checks | ||
GLOO_ENFORCE(opts.elementSize > 0); | ||
const auto recvRank = (context->size + context->rank - 1) % context->size; | ||
GLOO_ENFORCE(context->getPair(recvRank), "pair missing (rank ", recvRank, ")"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe rephrasing pair missing to something that is easier to be understood
// and the output buffer needs to be primed with the input. | ||
if (in != nullptr) { | ||
memcpy( | ||
(uint8_t*) out->ptr + context->rank * opts.inElements * opts.elementSize, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use C++ case, please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pietern has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Summary: This implements a ring reduction resulting the result scattered across processes, followed by a gather operation to the root process. The halving/doubling implementation is slated to be ported to this stateless style later and provided as an option through RedudeOptions. Rebase needed after #137 is merged. Pull Request resolved: #140 Reviewed By: manojkris Differential Revision: D10376422 Pulled By: pietern fbshipit-source-id: 2f32f693b8437cdff82a6528f18e576966984395
…age (facebookincubator#137) Summary: X-link: facebook/CacheLib#137 Fix error: anonymous non-C-compatible type given name for linkage purposes by alias declaration; add a tag name here [-Werror,-Wnon-c-typedef-for-linkage] Reviewed By: philippv Differential Revision: D36043476 fbshipit-source-id: c315a11e863b413a7662a497a3db71147488e782
…age (#137) Summary: X-link: facebook/CacheLib#137 Fix error: anonymous non-C-compatible type given name for linkage purposes by alias declaration; add a tag name here [-Werror,-Wnon-c-typedef-for-linkage] Reviewed By: philippv Differential Revision: D36043476 fbshipit-source-id: df3fa684c8655184dd8f87045542cd7f5704a65e
Like gather in the previous commit, this is a single function and
doesn't hold on to any state. This implements a ring allgather.
Rebase needed after #136 is merged.