Is "synchronizations" too broad in the description of SHMEM_TEAM_WORLD? (see 2a4102f)
Relation of SHMEM_TEAM_WORLD to MPI Sessions?
Is SHMEM_TEAM_WORLD as encompassing "all PEs" too restrictive for a future model in which not all processes in the job are active PEs in a SHMEM application?
Suggest: Adapt wording from SHMEM_CTX_INVALID for SHMEM_TEAM_NULL; specifically, comparing against SHMEM_TEAM_NULL does not necessarily indicate a valid team handle. (see 3e2a38b)
Suggest: SHMEM_TEAM_NULL → SHMEM_TEAM_INVALID (see dc57155)
Error Handling
The minimal changes necessary for the Teams proposal would be to define error modes only for those new routines that may return errors. This section goes beyond the scope of the Teams proposal itself. (@jdinan will try to follow up with specific feedback.)
Suggest: "~remote memory~ RMA operations"
Suggest: "...might generate error messages ~or~ and abort the application..." (topic for future discussion)
Team Management Routines (see 4dcc9e5)
Team Handles and Predefined Teams
Current text:
Team handles have local semantics only. That is, team handles should not be stored in shared variables and used across other PEs. Doing so will result in undefined behavior.
Suggest:
Team handles are not remotely accessible objects
Suggest removing "By default, OpenSHMEM creates predefined teams that will be available for use once the routine shmem_init has been called."
Rationale: The "available for use" restriction on SHMEM_TEAM_WORLD may contradict is use as a static initializer. No OpenSHMEM routine can be called before shmem_init[_thread] after all.
Team Objects and Multithreading Within a PE
Discussion re: locking within implementation vs. in application for concurrent accesses of a team handle.
Suggest: shmem_team_my_pe and shmem_team_n_pes (and others?) should actually be thread-safe routines.
Should only the collective team routines require serialized accesses?
Team Objects and Collective Ordering across PEs
Editorial: "...a team object encapsulates resources ~uses~ used to communicate..."
Suggest: "to ensure ~a consistent~ the same ordering"
Suggest referencing (vs. restating) restriction in "There is no need ... common parent team."
Team Creation
Is "Upon completion of ... parent or child teams." necessary?
shmem_team_my_pe and shmem_team_n_pes (see be43a4b)
Add math formatting for N and N-1
Reconcile argument description of "a valid OpenSHMEM team handle" with well-defined return values for SHMEM_TEAM_NULL.
Remove "All PEs in the team will get back the same value for the team size."
Move "For the team SHMEM_TEAM_WORLD, this will return the same value as shmem_my_pe." to "Notes" and/or add to shmem_my_pe that it is equivalent to shmem_team_my_pe(SHMEM_TEAM_WORLD).
shmem_team_config_t (see 286aa84)
Spelling: "existance", "guaruntee"
Can this structure have vendor-specific extension fields?
Suggest removing "These contexts may be created in any number of threads."
"best effort" vs. "hint"
What is the configuration of SHMEM_TEAM_WORLD?
Suggestions re: default and user-provided values
Specify that the default value of num_contexts be implementation defined
Provide a value indicating the user is requesting an "unspecified" number of contexts
Explicitly specify whether shmem_team_get_config returns the "unspecified" sentinel value or the actual value for which the team was configured
Suggest: field that means team only supports collectives
shmem_team_get_config (see 0034139)
Clarify all three states for team; i.e., valid, invalid andSHMEM_TEAM_NULL, or invalid and notSHMEM_TEAM_NULL
Add int return type indicating whether the config argument was modified (i.e., team is valid → return 0) or not (return nonzero)
shmem_team_translate_pe (see a866e47, 03ea9a0)
Rename section title to match function name; i.e., shmem_team_translate → shmem_team_translate_pe
Do not return -1 for invalid teams ("Return values"); leave as undefined behavior (as in "API description")
Move "If SHMEM_TEAM_WORLD is provided..." to "Notes" section
Example
Editorial: comment add "d" to shmem_team_split_stride
Remove #include <stdio.h>
Revise main(int argc, char *argv[]) to main(void)
Revise indentation, naming (rank vs. my_pe)
npes is undeclared
npes / 2 → (npes + 1) / 2
shmem_team_split_strided (see 10266ea, 8946910, 6691bd3)
Remove SHMEM_TEAM_WORLD reference in API arguments
Missing ending period from PE_start description
Update config from INOUT to IN, and addconstqualifier toconfig` argument
"processes" → "PEs"
Remove "None of the parameters need to reside in symmetric memory."
Reconcile invalid teams vs. SHMEM_TEAM_NULL
Specify that the routine is collective across the parent team
Refer config_mask description to appropriate section
Clarify validity of PE team triplet more explicitly; e.g., in LaTeX math
Clarify that all PEs in the newly created team must receive a valid team, or must all fail with SHMEM_TEAM_NULL
Clarify return value
If new_team is created successfully, is a zero value returned to all PEs in parent_team or only those in new_team?
If new_team cannot be created by any PE (e.g., due to resource constraints), do all PEs return a nonzero value?
Allow config to be a null pointer if config_mask is zero
Suggest: removing this example or combining with shmem_team_translate_pe example
shmem_team_split_2d
Fix double parentheses (see 6bd587b)
Remove comment about parameters in symmetric memory (see 02d31e1)
Fix double use of xaxis_mask (see 6bd587b)
Fix association of xteam and yteam with x and y coordinate; see "Notes"
Define yrange (its definition is provided, just not given as yrange)
Under "Notes", 7 teams, not 12
Example
Revise example to "work" with any number of PEs, though some might be degenerate cases
Fix/clarify calculation of zdim
shmem_team_destroy currently takes a pointer to shmem_team_t, so it should be shmem_team_destroy(&yzteam); we may change this behavior of shmem_team_destroy
Add return 0;
Reformat explanatory comments (prose and table) into LaTeX (not example C comments)
shmem_team_destroy (see 65faf2b)
Suggest: "all internal memory structures" → "all resources"
Should team be IN argument and not set to SHMEM_TEAM_NULL (consistent with shmem_ctx_destroy), or leave as is?
"erroneous" → "undefined" for destroying SHMEM_TEAM_WORLD
Require destruction of contexts on a team before that team's destruction or use reference counting on the team itself.
Suggest: The team could automatically destroy shared contexts, but not private contexts.
Suggest: Add that shmem_finalize destroys all teams and associated shared contexts.
Communication Management Routines
Intro
Make usage of "default team" / SHMEM_TEAM_WORLD" and "default context" /SHMEM_CTX_DEFAULT` consistent.
Suggest: "default team" → "world team"
shmem_ctx_create (see 5c1a003)
Why "initial" in "initial association"? Remove "initial" or replace with "fixed"?
shmem_team_create_ctx (see #127)
X-ref suggest: shmem_team_destroy should destroy all shared contexts associated with this team
Remove "All explicitly created resources associated with a team must be destroyed before the shmem_team_destroy routine is called." or make it explicitly about contexts.
shmem_ctx_destroy
What does "locally created context" mean? (see 9efe2a2)
Missing "added text" highlighting of "or shmem_team_create_ctx". (see b2f2200)
shmem_ctx_get_team (see 87f4f63)
The "if team is a null pointer" behavior should be undefined.
Add exception when ctx == SHMEM_CTX_INVALID
Example
in my_ctx_translate_pe: dest_pe → dest_team
int main() → int main(void)
Check/fix team size calculation
Don't start variables with leading underscore
Add return 0;
Comment: This is a complicated example. Is this a pattern of interest? Or, should this example be simplified?
Remote Memory Access Routines (see 5bfa27c)
Suggest: "If no context is passed to the routine, then the routine operates on the default context, which implies that the PE number is relative to the default team and is the global PE number."
Collective Routines
Intro (see 7646331)
Remove "These routines will be the standard for OpenSHMEM moving forward."
Suggest: Add "deprecated" markings around the active set-based collective item
Remove: "and will be phased out of implementations moving forward."
SHMEM_TEAM_WORLD? (see 2a4102f)SHMEM_TEAM_WORLDto MPI Sessions?SHMEM_TEAM_WORLDas encompassing "all PEs" too restrictive for a future model in which not all processes in the job are active PEs in a SHMEM application?SHMEM_CTX_INVALIDforSHMEM_TEAM_NULL; specifically, comparing againstSHMEM_TEAM_NULLdoes not necessarily indicate a valid team handle. (see 3e2a38b)SHMEM_TEAM_NULL→SHMEM_TEAM_INVALID(see dc57155)Team Management Routines (see 4dcc9e5)
Team Handles and Predefined Teams
Current text:
Suggest:
Suggest removing "By default, OpenSHMEM creates predefined teams that will be available for use once the routine shmem_init has been called."
SHMEM_TEAM_WORLDmay contradict is use as a static initializer. No OpenSHMEM routine can be called beforeshmem_init[_thread]after all.Team Objects and Multithreading Within a PE
Discussion re: locking within implementation vs. in application for concurrent accesses of a team handle.
Suggest:
shmem_team_my_peandshmem_team_n_pes(and others?) should actually be thread-safe routines.Should only the collective team routines require serialized accesses?
Team Objects and Collective Ordering across PEs
Editorial: "...a team object encapsulates resources ~uses~ used to communicate..."
Suggest: "to ensure ~a consistent~ the same ordering"
Consistency: "application" → "user program" (@manjugv?)
Suggest referencing (vs. restating) restriction in "There is no need ... common parent team."
Team Creation
Is "Upon completion of ... parent or child teams." necessary?
shmem_team_my_peandshmem_team_n_pes(see be43a4b)Add math formatting for N and N-1
Reconcile argument description of "a valid OpenSHMEM team handle" with well-defined return values for
SHMEM_TEAM_NULL.Remove "All PEs in the team will get back the same value for the team size."
Move "For the team SHMEM_TEAM_WORLD, this will return the same value as shmem_my_pe." to "Notes" and/or add to
shmem_my_pethat it is equivalent toshmem_team_my_pe(SHMEM_TEAM_WORLD).shmem_team_config_t(see 286aa84)Spelling: "existance", "guaruntee"
Can this structure have vendor-specific extension fields?
Suggest removing "These contexts may be created in any number of threads."
"best effort" vs. "hint"
What is the configuration of
SHMEM_TEAM_WORLD?Suggestions re: default and user-provided values
num_contextsbe implementation definedshmem_team_get_configreturns the "unspecified" sentinel value or the actual value for which the team was configuredSuggest: field that means team only supports collectives
shmem_team_get_config(see 0034139)Clarify all three states for
team; i.e., valid, invalid andSHMEM_TEAM_NULL, or invalid and notSHMEM_TEAM_NULLAdd
intreturn type indicating whether theconfigargument was modified (i.e., team is valid → return 0) or not (return nonzero)shmem_team_translate_pe(see a866e47, 03ea9a0)Rename section title to match function name; i.e.,
shmem_team_translate→shmem_team_translate_peDo not return -1 for invalid teams ("Return values"); leave as undefined behavior (as in "API description")
Move "If
SHMEM_TEAM_WORLDis provided..." to "Notes" sectionExample
shmem_team_split_stride#include <stdio.h>main(int argc, char *argv[])tomain(void)rankvs.my_pe)npesis undeclarednpes / 2→(npes + 1) / 2shmem_team_split_strided(see 10266ea, 8946910, 6691bd3)Remove
SHMEM_TEAM_WORLDreference in API argumentsMissing ending period from
PE_startdescriptionUpdate
configfromINOUTtoIN, and addconstqualifier toconfig` argument"processes" → "PEs"
Remove "None of the parameters need to reside in symmetric memory."
Reconcile invalid teams vs.
SHMEM_TEAM_NULLSpecify that the routine is collective across the parent team
Refer
config_maskdescription to appropriate sectionClarify validity of PE team triplet more explicitly; e.g., in LaTeX math
Clarify that all PEs in the newly created team must receive a valid team, or must all fail with
SHMEM_TEAM_NULLClarify return value
new_teamis created successfully, is a zero value returned to all PEs inparent_teamor only those innew_team?new_teamcannot be created by any PE (e.g., due to resource constraints), do all PEs return a nonzero value?Allow
configto be a null pointer ifconfig_maskis zeroSuggest: removing this example or combining with
shmem_team_translate_peexampleshmem_team_split_2dFix double parentheses (see 6bd587b)
Remove comment about parameters in symmetric memory (see 02d31e1)
Fix double use of
xaxis_mask(see 6bd587b)Fix association of
xteamandyteamwith x and y coordinate; see "Notes"Define
yrange(its definition is provided, just not given asyrange)Under "Notes", 7 teams, not 12
Example
zdimshmem_team_destroycurrently takes a pointer toshmem_team_t, so it should beshmem_team_destroy(&yzteam); we may change this behavior ofshmem_team_destroyreturn 0;shmem_team_destroy(see 65faf2b)Suggest: "all internal memory structures" → "all resources"
Should
teambeINargument and not set toSHMEM_TEAM_NULL(consistent withshmem_ctx_destroy), or leave as is?"erroneous" → "undefined" for destroying
SHMEM_TEAM_WORLDRequire destruction of contexts on a team before that team's destruction or use reference counting on the team itself.
shmem_finalizedestroys all teams and associated shared contexts.SHMEM_TEAM_WORLD" and "default context" /SHMEM_CTX_DEFAULT` consistent.shmem_ctx_create(see 5c1a003)shmem_team_create_ctx(see #127)shmem_team_destroyshould destroy all shared contexts associated with this teamshmem_team_destroyroutine is called." or make it explicitly about contexts.shmem_ctx_destroyshmem_team_create_ctx". (see b2f2200)shmem_ctx_get_team(see 87f4f63)teamis a null pointer" behavior should be undefined.ctx == SHMEM_CTX_INVALIDmy_ctx_translate_pe:dest_pe→dest_teamint main()→int main(void)return 0;and is the global PE number."SHMEM_TEAM_WORLD→ "default team" (or "world team")shmem_barrier_all()(see 41fa466, ee7c597)