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 tests for IBPUT/IBGET #35

Merged

Conversation

philipmarshall21
Copy link
Collaborator

This PR adds tests for the IBPUT/IBGET APIs (currently shmemx extensions, but ratified and to be included in a later OpenSHMEM spec version).

@philipmarshall21 philipmarshall21 requested a review from wrrobin March 7, 2024 22:44
@philipmarshall21 philipmarshall21 marked this pull request as ready for review March 7, 2024 22:44
@philipmarshall21
Copy link
Collaborator Author

@wrrobin It may be good to add Bryan, Mark, and Parker as contributors to this repo as well

@wrrobin
Copy link
Collaborator

wrrobin commented Mar 15, 2024

@philipmarshall21 - This looks good. One comment is that when we move these interfaces to shmem from shmemx, the test cases can be merged with c11_test_shmem_get/put.c and cxx_test_shmem_get/put.cpp rather than separate ones.

Copy link
Collaborator

@wrrobin wrrobin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved. Please take care of the minor comments.

@philipmarshall21
Copy link
Collaborator Author

@philipmarshall21 - This looks good. One comment is that when we move these interfaces to shmem from shmemx, the test cases can be merged with c11_test_shmem_get/put.c and cxx_test_shmem_get/put.cpp rather than separate ones.

Yep, totally agree!

@philipmarshall21
Copy link
Collaborator Author

Approved. Please take care of the minor comments.

Recommended changes have been applied. Not sure that I should merge just yet. The SOS CI would fail when trying to build the new tests because the function declarations are not present in shmemx.h (except in the SOS PR that adds ibput/ibget implementation). May be best to wait until SOS PR 1114 is reviewed and ready to be merged. Then we could merge this PR, run the CI on PR 1114 once more and merge it

@davidozog davidozog self-requested a review April 17, 2024 18:39
Copy link
Collaborator

@davidozog davidozog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests do not cover the API well because dest stride, source stride, block size, and num blocks are all constant/trivial. I think we can pretty easily add basic checks here, but it's ok to punt on it for now.

@philipmarshall21 philipmarshall21 merged commit e0f48a6 into openshmem-org:main May 1, 2024
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.

3 participants