Skip to content

predefined MPI object padding: set to fixed number of bytes #3634

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

Merged
merged 1 commit into from
Jun 1, 2017

Conversation

jsquyres
Copy link
Member

@jsquyres jsquyres commented Jun 1, 2017

Convert the predefined MPI object padding to a fixed number of bytes (vs. a multiple of sizeof(void*)) so that the padding is the same size between 32 and 64 bit builds. I.e., we won't have a situation where we've run out of padding in 32 bit builds but still have more space available in 64 bit builds.

Fixes #3610

Signed-off-by: Jeff Squyres [email protected]

@bwbarrett @hppritcha This is an ABI-changing commit (albeit pretty low risk). If we want this for v3.0.x, it needs to be in for v3.0.0.

Convert the predefined MPI object padding to a fixed number of bytes
(vs. a multiple of sizeof(void*)) so that the padding is the same size
between 32 and 64 bit builds.  I.e., we won't have a situation where
we've run out of padding in 32 bit builds but still have more space
available in 64 bit builds.

Fixes open-mpi#3610

Signed-off-by: Jeff Squyres <[email protected]>
@jsquyres
Copy link
Member Author

jsquyres commented Jun 1, 2017

@bosilca I'm not 100% convinced that this is the correct solution. Yes, it gives us a fixed padding that is the same between 32 and 64 bit builds... but is that important?

I.e., is the real fix to #3610 just to increment communicator padding by N multiples of sizeof(void*)? My only (admittedly weak) reason for raising this issue is that by going the route in this PR (converting all the padding sizes to a raw number of bytes) is that we make the padding a bit bigger on 32 bit platforms than we really need to.

Thoughts?

@bosilca
Copy link
Member

bosilca commented Jun 1, 2017

I prefer this solution to having different paddings on 32 and 64 bits architectures to cover all the needs. If someone that cares about 32 bits proposes a better approach, we take it. Meanwhile, this is good enough.

@bosilca bosilca merged commit d520c24 into open-mpi:master Jun 1, 2017
@jsquyres jsquyres deleted the pr/fix-predefined-pad branch June 1, 2017 19:45
bosilca added a commit that referenced this pull request Jun 1, 2017
This patch fixes a missed case by 5b670a2 (PR #3634).

Signed-off-by: George Bosilca <[email protected]>
@jsquyres
Copy link
Member Author

jsquyres commented Jun 1, 2017

@bwbarrett @hppritcha Turns out that we don't need this PR (and 037a85a) in v3.0.0 because we haven't run out of (32-bit) padding yet in v3.0.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mysterious 32 bit MTT build failures on Absort
2 participants