Skip to content

coll/libnbc: fix the red_schain algo of ireduce with MPI_IN_PLACE #2881

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 30, 2017
Merged

coll/libnbc: fix the red_schain algo of ireduce with MPI_IN_PLACE #2881

merged 1 commit into from
Jan 30, 2017

Conversation

@ggouaillardet
Copy link
Contributor

@ggouaillardet ggouaillardet commented Jan 30, 2017

this fixes a regression introduced in 045d0c5

Fixes #2879

Signed-off-by: Gilles Gouaillardet gilles@rist.or.jp

(back-ported from commit 9bcadbd)

@ggouaillardet
coll/libnbc: fix the red_schain algo of ireduce with MPI_IN_PLACE
07c0b68 
this fixes a regression introduced in open-mpi/ompi@045d0c5

Fixes open-mpi#2879

Signed-off-by: Gilles Gouaillardet <gilles@rist.or.jp>

(back-ported from commit open-mpi/ompi@9bcadbd)
@ggouaillardet ggouaillardet added this to the v2.0.2 milestone Jan 30, 2017
@ggouaillardet ggouaillardet mentioned this pull request Jan 30, 2017
Closed
@hppritcha hppritcha modified the milestones: v2.0.3, v2.0.2 Jan 30, 2017
@hppritcha
Copy link
Member

hppritcha commented Jan 30, 2017

moving this to 2.0.3.

@hppritcha hppritcha modified the milestones: v2.0.2, v2.0.3 Jan 30, 2017
@jsquyres jsquyres requested a review from jjhursey January 30, 2017 16:24
@jsquyres
Copy link
Member

jsquyres commented Jan 30, 2017

After @hppritcha and I looked at this a little closer, we're not 100% sure, but we think @ggouaillardet's fix may be correct -- or, at least, it seems more correct than @gpaulsen's fix.

@gpaulsen @jjhursey Please review ASAP. This is blocking 2.0.2.

jjhursey
jjhursey approved these changes Jan 30, 2017
View reviewed changes
Copy link
Member

@jjhursey jjhursey left a comment

Choose a reason for hiding this comment

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

👍 This is good, and matches our internal patch. The original PR got the second 1/2 of the patch wrong. My fault for not catching it during review. Thanks for the fix!

@jsquyres
Copy link
Member

jsquyres commented Jan 30, 2017

@gpaulsen @markalle @jjhursey Josh shared with me in IM that this particular algorithm is only active for np <= 4 and msg_size >= 64K. The ibm/collective/ireduce_*c tests have much smaller message sizes, and therefore do not test this algorithm.

Can you please add a test to ibm/collective that explicitly tests this algorithm?

Thanks.

@jsquyres jsquyres merged commit e291d0e into open-mpi:v2.0.x Jan 30, 2017
@jsquyres jsquyres mentioned this pull request Jan 30, 2017
Merged
@jsquyres
Copy link
Member

jsquyres commented Jan 30, 2017

@ggouaillardet Disregard -- I now see your ibm/collective/[i]reduce_big_in_place.c from over the weekend. Thanks!

@gpaulsen
Copy link
Member

gpaulsen commented Jan 30, 2017

Verified, that's the correct commit. Sorry for the type-o (actually my fault, not Josh's).

gpaulsen
gpaulsen reviewed Jan 30, 2017
View reviewed changes
Copy link
Member

@gpaulsen gpaulsen left a comment

Choose a reason for hiding this comment

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

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@gpaulsen gpaulsen gpaulsen left review comments

@jjhursey jjhursey jjhursey approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@gpaulsen gpaulsen

@markalle markalle

Labels

bug RM approved Severity: blocker

Projects

None yet

Milestone

v2.0.2

Development

Successfully merging this pull request may close these issues.

6 participants

@ggouaillardet @hppritcha @jsquyres @gpaulsen @jjhursey @markalle