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

Signal set add APIs #1109

Closed
wants to merge 5 commits into from
Closed

Conversation

wrrobin
Copy link
Collaborator

@wrrobin wrrobin commented Jan 31, 2024

This PR adds the APIs proposed in openshmem-org/specification#489

Copy link
Member

@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.

Looks great, but maybe the tests should go into test/shmemx? We've recently organized the tests in that manner to better support other libraries that might not have newer APIs like this.

Also, I wonder if it would be easier to handle this change after our upcoming "tests-sos as a submodule" change in #1106... perhaps we can discuss offline.

@@ -700,6 +708,52 @@ shmem_signal_fetch(const uint64_t* sig_addr)
return val;
}

void SHMEM_FUNCTION_ATTRIBUTES
shmemx_signal_add(uint64_t *sig_addr, uint64_t signal, int pe)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should sig_addr be const? I know in the PR where this was added in the spec the change was suggested to remove the const modifier from sig_addr, but it looks like in the main branch of the OpenSHMEM spec repo it is still a const parameter

}

void SHMEM_FUNCTION_ATTRIBUTES
shmemx_signal_set(uint64_t *sig_addr, uint64_t signal, int pe)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same question about const-ness of sig_addr

@wrrobin
Copy link
Collaborator Author

wrrobin commented Apr 17, 2024

Stale. Closing and opening a new one.

@wrrobin wrrobin closed this Apr 17, 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.

4 participants