v2.0.0: Meet oshmem spec1.3 #1073
Conversation
…ature) Annex G: Version 1.3 Added volatile to remotely accessible pointer argument in SHMEM_WAIT See Sections 8.7.1
…ature) Annex G: Version 1.3 Added volatile to remotely accessible pointer argument in SHMEM_LOCK See Sections 8.9.1
…ead-only pointer argument) Annex G: Version 1.3 Added const to every read-only pointer argument
|
@igor-ivanov We are feature complete for v2.0.0. Deferring to v2.1.0. |
|
Test PASSed. |
|
@jsquyres isn't this an API change? |
|
@hppritcha and I discussed this at length on the phone, and we examined the code. Howard tells me that he talked to @jladd-mlnx a bunch about this PR this morning, and it's not just a simple new feature that should be deferred to v2.1.0. This PR is more about trying to make future v2.x.y releases be backwards compatible to v2.0.0 in terms of the OSHMEM API. In particular, this PR adds This PR actually shows good faith and attempts to plan to be backwards compatible with a future v2.1.x release:
Meaning: subject to @hppritcha's review questions about the code (he'll enter some shortly), this PR is, in principle, good to go for v2.0.0. I'll move the milestone back. |
|
We'd like this in the 2.x branch, and assume that the risk to non OSHMEM codes would be low. |
|
I reviewed the OSHMEM v1.3 specification side-by-side with this PR, and this looks complete. However, I would like at least one more set of eyes on it. It's a benign change, but absolutely critical, from a backwards compatibility perspective, that we get this right at 2.0.0. I'd like for our Principal Engineer, @yosefe to please review it very carefully. I'm giving my 👍 contingent upon @yosefe 's 👍 |
oshmem/shmem/c/shmem_g.c
Outdated
| SHMEM_TYPE_G(_int16, int16_t, shmemx) | ||
| SHMEM_TYPE_G(_int32, int32_t, shmemx) | ||
| SHMEM_TYPE_G(_int64, int64_t, shmemx) | ||
| SHMEM_TYPE_GX(_int16, int16_t, shmemx) |
There was a problem hiding this comment.
maybe should add constant to extensions as well?
There was a problem hiding this comment.
@igor-ivanov let's change the extensions to use 'const' as well
There was a problem hiding this comment.
@yosefe keep in mind that spec1.3 tells nothing about ext API.
Seems to be consistent reduce extensions API should be changed too. Right? (see
2e11e8f#diff-475037a625962c3ff765b9247497f712R241)
|
please make sure version constant (SHMEM_MINOR_VERSION) is update after all features are in |
|
This looks ok to me.... |
|
Please also merge this on master before pushing to v2.0.0 (i.e., these kinds of code reviews and iterations should occur on master before pushing to release branches). I think you have a PR open on master for this issue: open-mpi/ompi#1542 -- probably should move review/iteration work over there, and then update this PR with the result of that. |
|
Roger that, @jsquyres |
|
@igor-ivanov please make the changes @yosefe recommended in master. @yosefe Let's move the review to the master. |
|
We'll pull this one in once PR open-mpi/ompi#1542 is merged in a cooks for a nite or two. Please not my comment on that PR about the |
|
spoke with @jladd-mlnx and he reminded me there are other things that need to go in to be SHMEM API 1.3 compliant - which will not go in till 2.1 release. So please ignore my comment. |
openshmem.org specification does not mention about extension api but there is an agreemnet to do these changes for related ex api too. see Annex G: Version 1.3 Added const to every read-only pointer argument
|
Test FAILed. |
|
this PR is updated to meet related PR in trunk (minor version was not updated) |
|
Test PASSed. |
|
Test PASSed. |
|
Looks like this is done on master (open-mpi/ompi#1542), so I think we're good to go here. |
9 participants
trunk: open-mpi/ompi#1542
:bot:assign: @hppritcha
:bot🏷️enhancement
:bot:milestone:v2.0.0
@jladd-mlnx please check