src: Fix bug in shmem_team_split_2d

[philipmarshall21]

PR #1153 adds a fix to shmem_team_split_strided to interpret the stride argument provided to the API as being relative to the parent team (i.e. consistent with OpenSHMEM spec).

The fix was implemented in shmem_internal_team_split_strided, which is also utilized by shmem_internal_split_2d. However, shmem_internal_team_split_2d was already passing parent team-relative stride values to its calls to shmem_internal_team_split_strided, so it is now being doubly converted following the recent changes.

[philipmarshall21]

Hey @davidozog! I think this is the fix needed for shmem_team_split_2d, but would need to test to be sure,

[philipmarshall21]

I think it is working as expected:

In the case above the "2d split failed" message is always from PEs 1 and 3 (i.e. odd-valued PEs), which is expected as even_team will be SHMEM_TEAM_INVALID on those PEs. It may be good to only perform the 2d split on even_team for even-numbered PEs (so check even_team != SHMEM_TEAM_INVALID prior to calls to check_2d(even_team, ...)) unless we are intentionally testing that ret is non-zero in such cases. Though if that is the case maybe there should some way to indicate if the split fail is expected because right now the branch in check_2d that prints that message does not increment errors, but I can imagine if there is a broken implementation of shmem_team_split_2d you could encounter a case where the operation fails when it is not expected to fail.

[philipmarshall21]

I think it is working as expected:

In the case above the "2d split failed" message is always from PEs 1 and 3 (i.e. odd-valued PEs), which is expected as even_team will be SHMEM_TEAM_INVALID on those PEs. It may be good to only perform the 2d split on even_team for even-numbered PEs (so check even_team != SHMEM_TEAM_INVALID prior to calls to check_2d(even_team, ...)) unless we are intentionally testing that ret is non-zero in such cases. Though if that is the case maybe there should some way to indicate if the split fail is expected because right now the branch in check_2d that prints that message does not increment errors, but I can imagine if there is a broken implementation of shmem_team_split_2d you could encounter a case where the operation fails when it is not expected to fail.

I've confirmed that putting check_2d(even_team, ...) calls inside an if (even_team != SHMEM_TEAM_INVALID) conditional does prevent "2d split failed" message from being printed

[davidozog]

I've confirmed that putting check_2d(even_team, ...) calls inside an if (even_team != SHMEM_TEAM_INVALID) conditional does prevent "2d split failed" message from being printed

Yes, I think this is expected/intended. The spec behavior we are working with is:

If parent_team compares equal to SHMEM_TEAM_INVALID then no new teams will be created, both xaxis_team and yaxis_team will be assigned the value SHMEM_TEAM_INVALID, and shmem_team_split_2d will return an nonzero value.

Thanks!