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

Add new style allgather collective #137

Closed
wants to merge 2 commits into from

Conversation

pietern
Copy link
Contributor

@pietern pietern commented Oct 8, 2018

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.

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.
Copy link
Contributor

@teng-li teng-li left a 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, ")");
Copy link
Contributor

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,
Copy link
Contributor

Choose a reason for hiding this comment

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

Use C++ case, please

Copy link

@facebook-github-bot facebook-github-bot left a 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.

facebook-github-bot pushed a commit that referenced this pull request Oct 23, 2018
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
luciang added a commit to luciang/gloo that referenced this pull request Apr 30, 2022
…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
facebook-github-bot pushed a commit that referenced this pull request Apr 30, 2022
…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
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