[c11] define mono atomics in terms of standard atomics #91489
Conversation
lambdageek
force-pushed
the
mono-c11-atomic
branch
from
d336ab5
to
1cce3f2
Compare
This comment was marked as outdated.
This comment was marked as outdated.
src/mono/mono/utils/atomic.h
Outdated
| #define MONO_IGNORE_STDATOMIC 1 | ||
| #endif | ||
|
|
||
| #if defined(HOST_ANDROID) && (defined(HOST_X86) || defined(HOST_AMD64) || defined(HOST_ARM)) |
There was a problem hiding this comment.
I think the opt-in setup would make more sense here
There was a problem hiding this comment.
I'm not so sure. I would expect the number of platforms that don't support stdatomic to decrease over time.
I can try it both ways. (And I want to move this decision to cmake to make it more visible)
on windows libclang.dll is in bin, not lib
ATOMIC_LONG_LONG_LOCK_FREE is 1, not 2
lambdageek
force-pushed
the
mono-c11-atomic
branch
from
9c73abd
to
4f29f31
Compare
Default to off
|
Oh right, moving the logic for |
This reverts commit da6b965.
|
/azp run runtime-community |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime-community |
|
Azure Pipelines successfully started running 1 pipeline(s). |
src/mono/mono/utils/atomic.h
Outdated
| mono_atomic_cas_i32 (volatile gint32 *dest, gint32 exch, gint32 comp) | ||
| { | ||
| g_static_assert (sizeof(atomic_int) == sizeof(*dest) && ATOMIC_INT_LOCK_FREE == 2); | ||
| (void)atomic_compare_exchange_strong ((atomic_int*)dest, &comp, exch); |
There was a problem hiding this comment.
Do we have strong guarantees for mono_atomic_cas_* functions? I believe we normally call these in loops to handle potential spuriously fails on platforms that could trigger them. If we would like to make the default implementation using strong guarantees, then maybe we should also offer the option to use weak when we already do cas in loops to potentially be more performant on platforms that takes advantage of spuriously fails?
There was a problem hiding this comment.
yea good point. I need to look at the callers and check if they're all in loops. My feeling is that we should make weak the default and add strong as a second option. I didn't want to change the mono functions as part of this PR (and strong seemed like a safer default). But I can audit and report back and then we can add the non-default versions
There was a problem hiding this comment.
Found 173 occurrences of mono_atomic_cas in src/mono/mono. Looked through about 2/3rds of them, chasing down callers if it's in a static helper method or a macro. About half are weak uses and about half are strong. So strong is definitely the right default. In a few places we could switch over to weak (particularly inside sgen there are some loops that could use weak). But it should be follow-up work, IMO.
There was a problem hiding this comment.
That is what I expected, we have made different assumption around the strong/weak guarantees around the runtime over years. Moving over to strong will potential correct some false assumptions previously made in runtime (at least it won't break anything), so it should be safe, and I agree that we could identify and use weak versions where it might benefit.
There was a problem hiding this comment.
Created #91747 for follow-up work
Co-authored-by: Johan Lorensson <[email protected]>
dotnet#91489)" (dotnet#91812)" This reverts commit 2157e37.
ghost
locked as resolved and limited conversation to collaborators
Contributes to #90404