Atomics: default to GCC builtin atomics and use C11 _Atomic as fall-back #10613
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The C11 atomics require annotating types with _Atomic so assignment involves sequential consistency. The overhead can be significant and defeats the purpose of OPAL_THREAD_*, which try to replace atomic operations with non-atomic operations if multi-threading is not enabled. This commit replaces C11 atomics with the GCC builtins (`__atomic_*`) and uses C11 as a fallback. It also fixes a couple of issues in the builtin atomic backend. Signed-off-by: Joseph Schuchart <[email protected]>
This was referenced Aug 16, 2022
|
bot:recheck |
bwbarrett
requested changes
Aug 24, 2022
| @@ -36,7 +36,7 @@ | |||
| * | |||
| *********************************************************************/ | |||
|
|
|||
| #if defined(PLATFORM_ARCH_X86_64) && defined (__GNUC__) && !defined(__llvm) && (__GNUC__ < 6) | |||
| #if defined(PLATFORM_ARCH_X86_64) && defined(PLATFORM_COMPILER_GNU) && __GNUC__ < 8 | |||
There was a problem hiding this comment.
This should really be two commits (changing the default, and then changing the builtn code). I'm happy with the platform compiler change, but why are we changing the version of GCC that has the busted MB?
| @@ -193,26 +193,30 @@ static inline intptr_t opal_atomic_swap_ptr(opal_atomic_intptr_t *addr, intptr_t | |||
|
|
|||
| static inline void opal_atomic_lock_init(opal_atomic_lock_t *lock, int32_t value) | |||
| { | |||
| lock = value; | |||
| if (OPAL_ATOMIC_LOCK_UNLOCKED == value) { | |||
There was a problem hiding this comment.
This either feels unnecessary or incomplete. Either we need an atomic set here in both cases or neither case.
| int ret = __atomic_exchange_n(&lock, OPAL_ATOMIC_LOCK_LOCKED, | ||
| __ATOMIC_ACQUIRE | __ATOMIC_HLE_ACQUIRE); | ||
| if (OPAL_ATOMIC_LOCK_LOCKED == ret) { | ||
| bool ret = __atomic_test_and_set(&lock, OPAL_ATOMIC_LOCK_LOCKED, |
There was a problem hiding this comment.
This all should be in its own commit with its own justification. This code was used in production prior to the 5.0 series, so it feels like (without justification in a commit message or comment) this is changing things to change things.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
bot:aws:retest |
|
Replaced by #10721, closing |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As per the discussion in #10492 (#10492 (comment)) we want to replace the C11
_Atomicvariables with a custom wrapper around basic atomic types. However, this requires significant changes throughout the code base to wrap direct assignment and initialization in macros. We may not want to backport this to 5.0.x at such a late stage. Instead this PR provides some fixes for the GCC builtin atomics (__atomic) and makes them the default, with C11 atomics as a fallback. This can then be backported to 5.0.x before attempting the larger changes inmain.The underlying problem is that C11 atomics require us to annotate atomic variables with
_Atomicthat then makes direct assignment sequentially consistent, which in turn make initialization costly and defeats the purpose of theOPAL_THREAD_*macros that try to avoid atomic operations if possible. As an alternative, #10492 removes the_Atomicannotation of variables and adds casts to the atomic functions, which is a hack.Signed-off-by: Joseph Schuchart [email protected]