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

Add 2d split unit test #9

Merged

Conversation

jdinan
Copy link
Collaborator

@jdinan jdinan commented Dec 19, 2019

Here's a sketch of the unit test I had in mind for 2d split. It seems to work fine on the SHMEMX_TEAM_WORLD cases and crash on the even_team cases. It also fails with npes == 1 for both cases. I had expected this case to work, should it not?

Signed-off-by: James Dinan <[email protected]>
@jdinan jdinan requested a review from davidozog December 19, 2019 02:20
@jdinan
Copy link
Collaborator Author

jdinan commented Dec 21, 2019

There were, indeed, some bugs in my unit test. I think they are mostly sorted out now, and hopefully the new PE index calculation makes more sense.

@davidozog
Copy link
Owner

I'm still seeing some errors here...

@jdinan - It looks like the problem is that xrange needs to be greater than npes. I don't think this is clearly specified in the spec though... When xrange > npes, the 2D split doesn't make sense to me... For example, if npes is 1 and xrange 2, do you expect just 1 x team? Or do you expect 2 x teams with the 2nd one equal to SHMEM_TEAM_INVALID?

@jdinan
Copy link
Collaborator Author

jdinan commented Jan 7, 2020

Oh, interesting. What does the spec say about xrange? I think I was expecting that xrange > npes would just result in a single xteam that is not full, similar to what happens when npes % xrange > 0.

@davidozog
Copy link
Owner

Yeah, it does kinda make sense for the xteam to be "unfilled" in this case, just like some yteams would be unfilled when npes % xrange > 0. My reading/understanding of the spec is that xrange is allowed to be (any?) non-negative number, but it's not explicit that the xteam is unfilled when xrange < npes. Is that your interpretation?

For what it's worth, I think it'd be simpler to require that xrange be less than or equal to npes, but perhaps that's not users' preference. I'll make a ticket to discuss this on the spec repo.

@jdinan
Copy link
Collaborator Author

jdinan commented Jan 7, 2020

Given that this is currently allowed, would something simple like if (xrange > parent->size) xrange = parent->size at the beginning of 2d split support this case?

@davidozog
Copy link
Owner

davidozog commented Jan 7, 2020

I think a one-liner like that wouldn't work because it messes with the "size" of the xteam (xsize here). But I'm sure something relatively simple can be done. I'll try it out.

Ha, nevermind - yeah I think it'll work...

@davidozog davidozog merged commit 8df0a83 into davidozog:pr/teams_split_2d-fixes Jan 7, 2020
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.

2 participants