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

Generalize team split 2D example for any # of PEs #13

Merged
merged 1 commit into from
Apr 5, 2020

Conversation

davidozog
Copy link

This proposal changes the shmem_team_split_2D example program to support any number of PEs (it originally supported only 12 or more PEs).

It calculates the most "cube-like" configuration of xdim x ydim x zdim, given npes.

This new algorithm does not produce the same configuration as before when run with 16 PEs, so the accompanying explanation text needed to be changed. I went with 12 PEs there to simplify and take up a little less space.

@davidozog
Copy link
Author

During the threads WG @jdinan asked whether the original example is correct for 12 PEs or more (we seem to remember that it exhibited errors at some point).

@jdinan - I just checked against Sandia OpenSHMEM, and the original example works as expected with any number of PEs, as long as there are at least 12. If there were problems in the past, it is likely that they were caused by some bugs in SOS and subsequently fixed in this PR: Sandia-OpenSHMEM/SOS#920

@jdinan
Copy link
Owner

jdinan commented Apr 2, 2020

@davidozog I'm on the fence. Given that the current code seems to be working, we could push this update to 1.6. Interested to hear what others think.

@davidozog
Copy link
Author

@jdinan Either way is fine with me. I do think we can do better in 1.6 regardless of whether we incorporate these changes - and I'm happy to help with that.

@jdinan
Copy link
Owner

jdinan commented Apr 2, 2020

Section committee members @agrippa @wrrobin and @naveen-rn and @nspark what are your thoughts on this?

@agrippa
Copy link
Collaborator

agrippa commented Apr 2, 2020

Given that (1) this isn't a semantic change, (2) I think the example is still pretty easy to understand, and (3) it seems to me like an improvement in how useful and illustrative the example is, I'm in favor of merging this in 1.5. At least, I don't see any reason to defer to 1.6. Is there a downside to merging now rather than later that I'm missing?

@jdinan
Copy link
Owner

jdinan commented Apr 2, 2020

No downside; just being conservative on changes. Appreciate the voices of support. Let's go ahead and merge this for 1.5. @davidozog Is this ready to go?

@davidozog
Copy link
Author

@jdinan I do believe it's ready to go, but it would be ideal if someone could do a quick correctness check on the explanation of a 12-PE run (i.e, the new values in the tables). Perhaps you or @agrippa have already checked it...

@jdinan jdinan merged commit 41982f8 into jdinan:sec/ctx-teams Apr 5, 2020
jdinan pushed a commit that referenced this pull request Apr 5, 2020
…menter-sec

Reupdate full/partial completion to signal_wait_until
jdinan pushed a commit that referenced this pull request Apr 6, 2020

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Clarified changelog for Teams
jdinan pushed a commit that referenced this pull request Oct 4, 2023
jdinan pushed a commit that referenced this pull request Oct 4, 2023
section/collectives: replace default team with world team
jdinan pushed a commit that referenced this pull request Oct 11, 2024
…rding

Collectives section: fix inconsistency in calling preconditions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants