-
Notifications
You must be signed in to change notification settings - Fork 928
Add C11 atomic wrapper structs to prevent direct access and control memory ordering #13373
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
Conversation
Co-authored-by: devreal <[email protected]>
Co-authored-by: devreal <[email protected]>
…irect access Co-authored-by: devreal <[email protected]>
|
Hello! The Git Commit Checker CI bot found a few problems with this PR: d49a9f8: Complete C11 atomic wrapper implementation - worki...
f81e3ed: Fix additional atomic access issues and add volati...
3759195: Implement C11 atomic wrapper structs and fix direc...
0c0f801: Initial plan
Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks! |
2 similar comments
|
Hello! The Git Commit Checker CI bot found a few problems with this PR: d49a9f8: Complete C11 atomic wrapper implementation - worki...
f81e3ed: Fix additional atomic access issues and add volati...
3759195: Implement C11 atomic wrapper structs and fix direc...
0c0f801: Initial plan
Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks! |
|
Hello! The Git Commit Checker CI bot found a few problems with this PR: d49a9f8: Complete C11 atomic wrapper implementation - worki...
f81e3ed: Fix additional atomic access issues and add volati...
3759195: Implement C11 atomic wrapper structs and fix direc...
0c0f801: Initial plan
Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks! |
|
I'm not sure I have objections to the overall idea, but the patches put all the context about why the change in the PR message, which means it's effectively lost once merged. THe commit messages should have the important context. |
|
@devreal what do you want to do with this PR? |
|
Closing. I didn't expect Copilot to open a PR against the main repo. |
This PR implements a C11 atomic wrapper system that prevents direct access to atomic variables and provides explicit memory ordering control, addressing the overhead of sequential consistency.
Problem
OpenMPI currently uses C11
_Atomictypes directly via typedefs, which allows users to accidentally perform direct assignments that default to sequential consistency:Solution
The implementation wraps all
_Atomictypes in structs to prevent direct access and provides accessor functions with explicit memory ordering control:Key Changes
_Atomictypes are now wrapped in structs with.valuemembersmemory_order_relaxedto avoid seq_cst overheadAffected Components
opal_stdatomic.h,atomic_stdc.h)Verification
The wrapper successfully prevents compilation of direct atomic access:
This ensures all atomic operations use explicit memory ordering, eliminating accidental sequential consistency overhead while maintaining correctness.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.