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

SC/collectives: add change logs #10

Merged
merged 11 commits into from
Feb 26, 2020

Conversation

minsii
Copy link

@minsii minsii commented Feb 8, 2020

No description provided.

@minsii minsii force-pushed the pr/sc/collectives/changelogs branch from 3f7db5d to d96591c Compare February 9, 2020 03:20
@minsii
Copy link
Author

minsii commented Feb 9, 2020

This PR contains several fixes and change logs for the collective section. @nspark @jdinan @davidozog Please review.

content/backmatter.tex Outdated Show resolved Hide resolved
@@ -1,6 +1,6 @@
\apisummary{
Registers the arrival of a \ac{PE} at a synchronization point and suspends
execution until all other \acp{PE} in the default team arrive at a synchronization point. For multithreaded programs, execution is suspended
execution until all other \acp{PE} in the default team arrive at the synchronization point. For multithreaded programs, execution is suspended
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we replace "default team" with SHMEM_TEAM_WORLD or "world team"?

Copy link
Author

Choose a reason for hiding this comment

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

"default team" is used in several sections including teams, collectives, and memory management. It is defined in section 7. Library Handles.

I agree that SHMEM_TEAM_WORLD is more specific. Perhaps we want to replace it everywhere for consistency. Do we need to create separate commits for each SC? Is there a way that we can fix it in a single commit?

Copy link
Owner

Choose a reason for hiding this comment

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

I'd prefer "world team"—since, as with contexts, one could copy the handle to SHMEM_TEAM_WORLD and still refer to the same team—but this would be a "whole document" change (which I'd support).

Copy link
Author

@minsii minsii Feb 12, 2020

Choose a reason for hiding this comment

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

Good point on "world team". I created issue openshmem-org#371 to track the changes. I'd deal with the updates in separate commits and submit to the corresponding SC.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@nspark I'm fine with "world team" but we need to be precise about what that means. If one thread is calling shmem_malloc, is it a violation of the threading rules for another to call shmem_team_barrier(SHMEM_TEAM_WORLD) at the same time?

Copy link
Author

Choose a reason for hiding this comment

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

Thinking again about the example @nspark mentioned: one does handle = SHMEM_TEAM_WORLD; and uses handle in later calls. The value of handle is still the predefined SHMEM_TEAM_WORLD, and the function call reads the value but not &handle. Thus, I don't think people may confuse if the handle is a different team.

However, if we say "world team", I am afraid that people may confuse if a duplicated team of SHMEM_TEAM_WORLD (e.g., created by shmem_team_split_strided) is also a "world team".

In summary, I am now thinking SHMEM_TEAM_WORLD might be a better replacement for "default team." Does it resolve @jdinan's question, or avoid such confusion?

Copy link
Collaborator

Choose a reason for hiding this comment

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

In Section 7, you can clarify that SHMEM_TEAM_WORLD handle corresponds to the "world team". Currently it says:

Handle of type shmem_team_t that corresponds to the default team of all PEs in the OpenSHMEM program.

Copy link
Author

Choose a reason for hiding this comment

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

I am working on the default team->world team replacement now. It will be in separate PRs.

@minsii minsii force-pushed the pr/sc/collectives/changelogs branch from d96591c to eed3e45 Compare February 12, 2020 12:37
The reduction operators mentioned in the intro of active-set-based
collectives did not follow a specific order, i.e., neither alphabetized
nor grouped by operator types. This patch reorders it to match the
description of team-based collectives. The new order also matches that
of the operator definitions in section 9.11.7 SHMEM_REDUCTIONS.
@minsii minsii force-pushed the pr/sc/collectives/changelogs branch from eed3e45 to 55d39c4 Compare February 12, 2020 16:57
@minsii
Copy link
Author

minsii commented Feb 17, 2020

@nspark @jdinan @davidozog I think all comments should be fixed except the "default team"->"world team" replacement. I will address it in separate PRs. This PR should be ready for review & merge. Could you please check again?

Copy link
Owner

@nspark nspark 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; only requesting two minor formatting changes

content/backmatter.tex Outdated Show resolved Hide resolved
content/backmatter.tex Outdated Show resolved Hide resolved
Minor formatting tweaks
content/backmatter.tex Outdated Show resolved Hide resolved
Minor rewording, as team-based broadcasts are new, so we're not changing behavior, but it's worth calling out here that the behavior is different from that of active-set-based broadcasts.
@nspark nspark merged commit ac012a2 into nspark:section/collectives Feb 26, 2020
BryantLam added a commit to BryantLam/openshmem-specification that referenced this pull request Mar 31, 2020
BryantLam added a commit to BryantLam/openshmem-specification that referenced this pull request Mar 31, 2020
Merge conflict between:
- #12
- #13
- nspark#10

This entry will be moved into sec/backmatter:
  #16
nspark pushed a commit that referenced this pull request Apr 2, 2020
nspark pushed a commit that referenced this pull request Apr 8, 2020
Replace default team with world team
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.

3 participants