From f64f5ee8002425160f556d32b74c9064f2b0479c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Petryka?= Date: Tue, 27 Feb 2024 20:09:40 +0100 Subject: [PATCH 1/8] Convert small atomic fallbacks to managed --- .../System/Threading/Interlocked.CoreCLR.cs | 86 ----- src/coreclr/inc/winwrap.h | 9 - src/coreclr/nativeaot/Runtime/EHHelpers.cpp | 4 - .../nativeaot/Runtime/arm/Interlocked.S | 45 --- src/coreclr/nativeaot/Runtime/portable.cpp | 12 - .../src/System/Runtime/RuntimeImports.cs | 8 - .../src/System/Threading/Interlocked.cs | 54 --- .../src/System/Runtime/RuntimeImports.cs | 8 - .../src/System/Threading/Interlocked.cs | 20 -- src/coreclr/pal/inc/pal.h | 34 -- src/coreclr/vm/comutilnative.cpp | 32 -- src/coreclr/vm/comutilnative.h | 4 - src/coreclr/vm/ecalllist.h | 6 +- .../src/System/Threading/Interlocked.cs | 161 ++++++++- .../src/System/Threading/Interlocked.Mono.cs | 16 - src/mono/mono/metadata/icall-def.h | 4 - src/mono/mono/metadata/threads-types.h | 12 - src/mono/mono/metadata/threads.c | 32 -- src/mono/mono/utils/atomic.h | 91 +---- src/tests/JIT/Intrinsics/Interlocked.cs | 324 ++++++++++-------- 20 files changed, 337 insertions(+), 625 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Threading/Interlocked.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Threading/Interlocked.CoreCLR.cs index e2d0a033b5d0c7..93df3bdfed9fb0 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Threading/Interlocked.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Threading/Interlocked.CoreCLR.cs @@ -42,48 +42,6 @@ public static long Decrement(ref long location) => #endregion #region Exchange - /// Sets a 8-bit unsigned integer to a specified value and returns the original value, as an atomic operation. - /// The variable to set to the specified value. - /// The value to which the parameter is set. - /// The original value of . - /// The address of location1 is a null pointer. - [Intrinsic] - [MethodImpl(MethodImplOptions.AggressiveInlining)] - public static byte Exchange(ref byte location1, byte value) - { -#if TARGET_X86 || TARGET_AMD64 || TARGET_ARM64 - return Exchange(ref location1, value); // Must expand intrinsic -#else - if (Unsafe.IsNullRef(ref location1)) - ThrowHelper.ThrowNullReferenceException(); - return Exchange8(ref location1, value); -#endif - } - - [MethodImpl(MethodImplOptions.InternalCall)] - private static extern byte Exchange8(ref byte location1, byte value); - - /// Sets a 16-bit signed integer to a specified value and returns the original value, as an atomic operation. - /// The variable to set to the specified value. - /// The value to which the parameter is set. - /// The original value of . - /// The address of location1 is a null pointer. - [Intrinsic] - [MethodImpl(MethodImplOptions.AggressiveInlining)] - public static short Exchange(ref short location1, short value) - { -#if TARGET_X86 || TARGET_AMD64 || TARGET_ARM64 - return Exchange(ref location1, value); // Must expand intrinsic -#else - if (Unsafe.IsNullRef(ref location1)) - ThrowHelper.ThrowNullReferenceException(); - return Exchange16(ref location1, value); -#endif - } - - [MethodImpl(MethodImplOptions.InternalCall)] - private static extern short Exchange16(ref short location1, short value); - /// Sets a 32-bit signed integer to a specified value and returns the original value, as an atomic operation. /// The variable to set to the specified value. /// The value to which the parameter is set. @@ -162,50 +120,6 @@ public static T Exchange([NotNullIfNotNull(nameof(value))] ref T location1, T #endregion #region CompareExchange - /// Compares two 8-bit unsigned integers for equality and, if they are equal, replaces the first value. - /// The destination, whose value is compared with and possibly replaced. - /// The value that replaces the destination value if the comparison results in equality. - /// The value that is compared to the value at . - /// The original value in . - /// The address of is a null pointer. - [Intrinsic] - [MethodImpl(MethodImplOptions.AggressiveInlining)] - public static byte CompareExchange(ref byte location1, byte value, byte comparand) - { -#if TARGET_X86 || TARGET_AMD64 || TARGET_ARM64 - return CompareExchange(ref location1, value, comparand); // Must expand intrinsic -#else - if (Unsafe.IsNullRef(ref location1)) - ThrowHelper.ThrowNullReferenceException(); - return CompareExchange8(ref location1, value, comparand); -#endif - } - - [MethodImpl(MethodImplOptions.InternalCall)] - private static extern byte CompareExchange8(ref byte location1, byte value, byte comparand); - - /// Compares two 16-bit signed integers for equality and, if they are equal, replaces the first value. - /// The destination, whose value is compared with and possibly replaced. - /// The value that replaces the destination value if the comparison results in equality. - /// The value that is compared to the value at . - /// The original value in . - /// The address of is a null pointer. - [Intrinsic] - [MethodImpl(MethodImplOptions.AggressiveInlining)] - public static short CompareExchange(ref short location1, short value, short comparand) - { -#if TARGET_X86 || TARGET_AMD64 || TARGET_ARM64 - return CompareExchange(ref location1, value, comparand); // Must expand intrinsic -#else - if (Unsafe.IsNullRef(ref location1)) - ThrowHelper.ThrowNullReferenceException(); - return CompareExchange16(ref location1, value, comparand); -#endif - } - - [MethodImpl(MethodImplOptions.InternalCall)] - private static extern short CompareExchange16(ref short location1, short value, short comparand); - /// Compares two 32-bit signed integers for equality and, if they are equal, replaces the first value. /// The destination, whose value is compared with and possibly replaced. /// The value that replaces the destination value if the comparison results in equality. diff --git a/src/coreclr/inc/winwrap.h b/src/coreclr/inc/winwrap.h index 4cf0b9655ad1d3..6235e4b5a18119 100644 --- a/src/coreclr/inc/winwrap.h +++ b/src/coreclr/inc/winwrap.h @@ -254,13 +254,4 @@ WszCreateProcess( LPPROCESS_INFORMATION lpProcessInformation ); -#ifdef HOST_WINDOWS - -// -// Workaround for https://github.com/microsoft/WindowsAppSDK/issues/4074 -// Windows SDK is missing InterlockedCompareExchange8 definition. -// -#define InterlockedCompareExchange8 _InterlockedCompareExchange8 - -#endif // HOST_WINDOWS #endif // __WIN_WRAP_H__ diff --git a/src/coreclr/nativeaot/Runtime/EHHelpers.cpp b/src/coreclr/nativeaot/Runtime/EHHelpers.cpp index 28cb9e617f09bb..b136b9b5984e76 100644 --- a/src/coreclr/nativeaot/Runtime/EHHelpers.cpp +++ b/src/coreclr/nativeaot/Runtime/EHHelpers.cpp @@ -286,8 +286,6 @@ EXTERN_C CODE_LOCATION RhpCheckedAssignRefAVLocation; EXTERN_C CODE_LOCATION RhpCheckedLockCmpXchgAVLocation; EXTERN_C CODE_LOCATION RhpCheckedXchgAVLocation; #if !defined(HOST_AMD64) && !defined(HOST_ARM64) -EXTERN_C CODE_LOCATION RhpLockCmpXchg8AVLocation; -EXTERN_C CODE_LOCATION RhpLockCmpXchg16AVLocation; EXTERN_C CODE_LOCATION RhpLockCmpXchg32AVLocation; EXTERN_C CODE_LOCATION RhpLockCmpXchg64AVLocation; #endif @@ -312,8 +310,6 @@ static bool InWriteBarrierHelper(uintptr_t faultingIP) (uintptr_t)&RhpCheckedLockCmpXchgAVLocation, (uintptr_t)&RhpCheckedXchgAVLocation, #if !defined(HOST_AMD64) && !defined(HOST_ARM64) - (uintptr_t)&RhpLockCmpXchg8AVLocation, - (uintptr_t)&RhpLockCmpXchg16AVLocation, (uintptr_t)&RhpLockCmpXchg32AVLocation, (uintptr_t)&RhpLockCmpXchg64AVLocation, #endif diff --git a/src/coreclr/nativeaot/Runtime/arm/Interlocked.S b/src/coreclr/nativeaot/Runtime/arm/Interlocked.S index 631731c7e3a327..47b8bbeff00e95 100644 --- a/src/coreclr/nativeaot/Runtime/arm/Interlocked.S +++ b/src/coreclr/nativeaot/Runtime/arm/Interlocked.S @@ -7,51 +7,6 @@ #include // generated by the build from AsmOffsets.cpp #include -// WARNING: Code in EHHelpers.cpp makes assumptions about this helper, in particular: -// - Function "InWriteBarrierHelper" assumes an AV due to passed in null pointer will happen at RhpLockCmpXchg8AVLocation -// - Function "UnwindSimpleHelperToCaller" assumes no registers were pushed and LR contains the return address -// r0 = destination address -// r1 = value -// r2 = comparand -LEAF_ENTRY RhpLockCmpXchg8, _TEXT - dmb -GLOBAL_LABEL RhpLockCmpXchg8AVLocation -LOCAL_LABEL(CmpXchg8Retry): - ldrexb r3, [r0] - cmp r2, r3 - bne LOCAL_LABEL(CmpXchg8Exit) - strexb r12, r1, [r0] - cmp r12, #0 - bne LOCAL_LABEL(CmpXchg8Retry) -LOCAL_LABEL(CmpXchg8Exit): - mov r0, r3 - dmb - bx lr -LEAF_END RhpLockCmpXchg8, _TEXT - -// WARNING: Code in EHHelpers.cpp makes assumptions about this helper, in particular: -// - Function "InWriteBarrierHelper" assumes an AV due to passed in null pointer will happen at RhpLockCmpXchg16AVLocation -// - Function "UnwindSimpleHelperToCaller" assumes no registers were pushed and LR contains the return address -// r0 = destination address -// r1 = value -// r2 = comparand -LEAF_ENTRY RhpLockCmpXchg16, _TEXT - uxth r2, r2 - dmb -GLOBAL_LABEL RhpLockCmpXchg16AVLocation -LOCAL_LABEL(CmpXchg16Retry): - ldrexh r3, [r0] - cmp r2, r3 - bne LOCAL_LABEL(CmpXchg16Exit) - strexh r12, r1, [r0] - cmp r12, #0 - bne LOCAL_LABEL(CmpXchg16Retry) -LOCAL_LABEL(CmpXchg16Exit): - sxth r0, r3 - dmb - bx lr -LEAF_END RhpLockCmpXchg16, _TEXT - // WARNING: Code in EHHelpers.cpp makes assumptions about this helper, in particular: // - Function "InWriteBarrierHelper" assumes an AV due to passed in null pointer will happen at RhpLockCmpXchg32AVLocation // - Function "UnwindSimpleHelperToCaller" assumes no registers were pushed and LR contains the return address diff --git a/src/coreclr/nativeaot/Runtime/portable.cpp b/src/coreclr/nativeaot/Runtime/portable.cpp index 7ddb06baee5a87..9497a929b8f7f0 100644 --- a/src/coreclr/nativeaot/Runtime/portable.cpp +++ b/src/coreclr/nativeaot/Runtime/portable.cpp @@ -367,18 +367,6 @@ COOP_PINVOKE_HELPER(Object *, RhpCheckedXchg, (Object ** location, Object * valu return ret; } -COOP_PINVOKE_HELPER(uint8_t, RhpLockCmpXchg8, (uint8_t * location, uint8_t value, uint8_t comparand)) -{ - ASSERT_UNCONDITIONALLY("NYI"); - return 0; -} - -COOP_PINVOKE_HELPER(int16_t, RhpLockCmpXchg16, (int16_t * location, int16_t value, int16_t comparand)) -{ - ASSERT_UNCONDITIONALLY("NYI"); - return 0; -} - COOP_PINVOKE_HELPER(int32_t, RhpLockCmpXchg32, (int32_t * location, int32_t value, int32_t comparand)) { // @TODO: USE_PORTABLE_HELPERS - Null check diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/RuntimeImports.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/RuntimeImports.cs index 3f30983a74c3b2..b3c449d0d24894 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/RuntimeImports.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/RuntimeImports.cs @@ -821,14 +821,6 @@ internal static partial void NativeRuntimeEventSource_LogWaitHandleWaitStart( // // Interlocked helpers // - [MethodImplAttribute(MethodImplOptions.InternalCall)] - [RuntimeImport(RuntimeLibrary, "RhpLockCmpXchg8")] - internal static extern byte InterlockedCompareExchange(ref byte location1, byte value, byte comparand); - - [MethodImplAttribute(MethodImplOptions.InternalCall)] - [RuntimeImport(RuntimeLibrary, "RhpLockCmpXchg16")] - internal static extern short InterlockedCompareExchange(ref short location1, short value, short comparand); - [MethodImplAttribute(MethodImplOptions.InternalCall)] [RuntimeImport(RuntimeLibrary, "RhpLockCmpXchg32")] internal static extern int InterlockedCompareExchange(ref int location1, int value, int comparand); diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Threading/Interlocked.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Threading/Interlocked.cs index b4fd68cf7c7034..6b351011a00b20 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Threading/Interlocked.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Threading/Interlocked.cs @@ -11,26 +11,6 @@ public static partial class Interlocked { #region CompareExchange - [Intrinsic] - public static byte CompareExchange(ref byte location1, byte value, byte comparand) - { -#if TARGET_X86 || TARGET_AMD64 || TARGET_ARM64 - return CompareExchange(ref location1, value, comparand); -#else - return RuntimeImports.InterlockedCompareExchange(ref location1, value, comparand); -#endif - } - - [Intrinsic] - public static short CompareExchange(ref short location1, short value, short comparand) - { -#if TARGET_X86 || TARGET_AMD64 || TARGET_ARM64 - return CompareExchange(ref location1, value, comparand); -#else - return RuntimeImports.InterlockedCompareExchange(ref location1, value, comparand); -#endif - } - [Intrinsic] public static int CompareExchange(ref int location1, int value, int comparand) { @@ -70,40 +50,6 @@ public static T CompareExchange(ref T location1, T value, T comparand) where #region Exchange - [Intrinsic] - public static byte Exchange(ref byte location1, byte value) - { -#if TARGET_X86 || TARGET_AMD64 || TARGET_ARM64 - return Exchange(ref location1, value); -#else - byte oldValue; - - do - { - oldValue = location1; - } while (CompareExchange(ref location1, value, oldValue) != oldValue); - - return oldValue; -#endif - } - - [Intrinsic] - public static short Exchange(ref short location1, short value) - { -#if TARGET_X86 || TARGET_AMD64 || TARGET_ARM64 - return Exchange(ref location1, value); -#else - short oldValue; - - do - { - oldValue = location1; - } while (CompareExchange(ref location1, value, oldValue) != oldValue); - - return oldValue; -#endif - } - [Intrinsic] public static int Exchange(ref int location1, int value) { diff --git a/src/coreclr/nativeaot/Test.CoreLib/src/System/Runtime/RuntimeImports.cs b/src/coreclr/nativeaot/Test.CoreLib/src/System/Runtime/RuntimeImports.cs index 9bfc3314b2d3bb..539418fc76c308 100644 --- a/src/coreclr/nativeaot/Test.CoreLib/src/System/Runtime/RuntimeImports.cs +++ b/src/coreclr/nativeaot/Test.CoreLib/src/System/Runtime/RuntimeImports.cs @@ -90,14 +90,6 @@ internal static IntPtr RhGetModuleSection(TypeManagerHandle module, ReadyToRunSe // // Interlocked helpers // - [MethodImplAttribute(MethodImplOptions.InternalCall)] - [RuntimeImport(RuntimeLibrary, "RhpLockCmpXchg8")] - internal static extern byte InterlockedCompareExchange(ref byte location1, byte value, byte comparand); - - [MethodImplAttribute(MethodImplOptions.InternalCall)] - [RuntimeImport(RuntimeLibrary, "RhpLockCmpXchg16")] - internal static extern short InterlockedCompareExchange(ref short location1, short value, short comparand); - [MethodImplAttribute(MethodImplOptions.InternalCall)] [RuntimeImport(RuntimeLibrary, "RhpLockCmpXchg32")] internal static extern int InterlockedCompareExchange(ref int location1, int value, int comparand); diff --git a/src/coreclr/nativeaot/Test.CoreLib/src/System/Threading/Interlocked.cs b/src/coreclr/nativeaot/Test.CoreLib/src/System/Threading/Interlocked.cs index 12b3bb500d3a33..9ac6aa5110ad81 100644 --- a/src/coreclr/nativeaot/Test.CoreLib/src/System/Threading/Interlocked.cs +++ b/src/coreclr/nativeaot/Test.CoreLib/src/System/Threading/Interlocked.cs @@ -18,26 +18,6 @@ public static IntPtr CompareExchange(ref IntPtr location1, IntPtr value, IntPtr #endif } - [Intrinsic] - public static byte CompareExchange(ref byte location1, byte value, byte comparand) - { -#if TARGET_X86 || TARGET_AMD64 || TARGET_ARM64 - return CompareExchange(ref location1, value, comparand); -#else - return RuntimeImports.InterlockedCompareExchange(ref location1, value, comparand); -#endif - } - - [Intrinsic] - public static short CompareExchange(ref short location1, short value, short comparand) - { -#if TARGET_X86 || TARGET_AMD64 || TARGET_ARM64 - return CompareExchange(ref location1, value, comparand); -#else - return RuntimeImports.InterlockedCompareExchange(ref location1, value, comparand); -#endif - } - [Intrinsic] public static int CompareExchange(ref int location1, int value, int comparand) { diff --git a/src/coreclr/pal/inc/pal.h b/src/coreclr/pal/inc/pal.h index 4a0a341b2272ef..cb60519e727e8a 100644 --- a/src/coreclr/pal/inc/pal.h +++ b/src/coreclr/pal/inc/pal.h @@ -3636,20 +3636,6 @@ Return Values The function returns the initial value pointed to by Target. --*/ -Define_InterlockMethod( - CHAR, - InterlockedExchange8(IN OUT CHAR volatile *Target, CHAR Value), - InterlockedExchange8(Target, Value), - __atomic_exchange_n(Target, Value, __ATOMIC_ACQ_REL) -) - -Define_InterlockMethod( - SHORT, - InterlockedExchange16(IN OUT SHORT volatile *Target, SHORT Value), - InterlockedExchange16(Target, Value), - __atomic_exchange_n(Target, Value, __ATOMIC_ACQ_REL) -) - Define_InterlockMethod( LONG, InterlockedExchange(IN OUT LONG volatile *Target, LONG Value), @@ -3708,26 +3694,6 @@ Return Values The return value is the initial value of the destination. --*/ -Define_InterlockMethod( - CHAR, - InterlockedCompareExchange8(IN OUT CHAR volatile *Destination, IN CHAR Exchange, IN CHAR Comperand), - InterlockedCompareExchange8(Destination, Exchange, Comperand), - __sync_val_compare_and_swap( - Destination, /* The pointer to a variable whose value is to be compared with. */ - Comperand, /* The value to be compared */ - Exchange /* The value to be stored */) -) - -Define_InterlockMethod( - SHORT, - InterlockedCompareExchange16(IN OUT SHORT volatile *Destination, IN SHORT Exchange, IN SHORT Comperand), - InterlockedCompareExchange16(Destination, Exchange, Comperand), - __sync_val_compare_and_swap( - Destination, /* The pointer to a variable whose value is to be compared with. */ - Comperand, /* The value to be compared */ - Exchange /* The value to be stored */) -) - Define_InterlockMethod( LONG, InterlockedCompareExchange(IN OUT LONG volatile *Destination, IN LONG Exchange, IN LONG Comperand), diff --git a/src/coreclr/vm/comutilnative.cpp b/src/coreclr/vm/comutilnative.cpp index 6c7e2468d2744b..a3c9d0a848cdff 100644 --- a/src/coreclr/vm/comutilnative.cpp +++ b/src/coreclr/vm/comutilnative.cpp @@ -1463,22 +1463,6 @@ NOINLINE void GCInterface::GarbageCollectModeAny(int generation) #include -FCIMPL2(FC_UINT8_RET,COMInterlocked::Exchange8, UINT8 *location, UINT8 value) -{ - FCALL_CONTRACT; - - return (UINT8)InterlockedExchange8((CHAR *) location, (CHAR)value); -} -FCIMPLEND - -FCIMPL2(FC_INT16_RET,COMInterlocked::Exchange16, INT16 *location, INT16 value) -{ - FCALL_CONTRACT; - - return InterlockedExchange16((SHORT *) location, value); -} -FCIMPLEND - FCIMPL2(INT32,COMInterlocked::Exchange32, INT32 *location, INT32 value) { FCALL_CONTRACT; @@ -1495,22 +1479,6 @@ FCIMPL2_IV(INT64,COMInterlocked::Exchange64, INT64 *location, INT64 value) } FCIMPLEND -FCIMPL3(FC_UINT8_RET, COMInterlocked::CompareExchange8, UINT8* location, UINT8 value, UINT8 comparand) -{ - FCALL_CONTRACT; - - return (UINT8)InterlockedCompareExchange8((CHAR*)location, (CHAR)value, (CHAR)comparand); -} -FCIMPLEND - -FCIMPL3(FC_INT16_RET, COMInterlocked::CompareExchange16, INT16* location, INT16 value, INT16 comparand) -{ - FCALL_CONTRACT; - - return InterlockedCompareExchange16((SHORT*)location, value, comparand); -} -FCIMPLEND - FCIMPL3(INT32, COMInterlocked::CompareExchange32, INT32* location, INT32 value, INT32 comparand) { FCALL_CONTRACT; diff --git a/src/coreclr/vm/comutilnative.h b/src/coreclr/vm/comutilnative.h index 0f305e0af90072..3e64207564c847 100644 --- a/src/coreclr/vm/comutilnative.h +++ b/src/coreclr/vm/comutilnative.h @@ -229,12 +229,8 @@ extern "C" uint64_t QCALLTYPE GCInterface_GetGenerationBudget(int generation); class COMInterlocked { public: - static FCDECL2(FC_UINT8_RET, Exchange8, UINT8 *location, UINT8 value); - static FCDECL2(FC_INT16_RET, Exchange16, INT16 *location, INT16 value); static FCDECL2(INT32, Exchange32, INT32 *location, INT32 value); static FCDECL2_IV(INT64, Exchange64, INT64 *location, INT64 value); - static FCDECL3(FC_UINT8_RET, CompareExchange8, UINT8* location, UINT8 value, UINT8 comparand); - static FCDECL3(FC_INT16_RET, CompareExchange16, INT16* location, INT16 value, INT16 comparand); static FCDECL3(INT32, CompareExchange32, INT32* location, INT32 value, INT32 comparand); static FCDECL3_IVV(INT64, CompareExchange64, INT64* location, INT64 value, INT64 comparand); static FCDECL2(LPVOID, ExchangeObject, LPVOID* location, LPVOID value); diff --git a/src/coreclr/vm/ecalllist.h b/src/coreclr/vm/ecalllist.h index 9da9ebdb102a6f..766c07fb60cde2 100644 --- a/src/coreclr/vm/ecalllist.h +++ b/src/coreclr/vm/ecalllist.h @@ -239,7 +239,7 @@ FCFuncStart(gCOMFieldHandleNewFuncs) FCFuncElement("GetLoaderAllocator", RuntimeFieldHandle::GetLoaderAllocator) FCFuncElement("IsFastPathSupported", RuntimeFieldHandle::IsFastPathSupported) FCFuncElement("GetInstanceFieldOffset", RuntimeFieldHandle::GetInstanceFieldOffset) - FCFuncElement("GetStaticFieldAddress", RuntimeFieldHandle::GetStaticFieldAddress) + FCFuncElement("GetStaticFieldAddress", RuntimeFieldHandle::GetStaticFieldAddress) FCFuncEnd() FCFuncStart(gCOMModuleHandleFuncs) @@ -437,13 +437,9 @@ FCFuncStart(gInteropMarshalFuncs) FCFuncEnd() FCFuncStart(gInterlockedFuncs) - FCFuncElement("Exchange8", COMInterlocked::Exchange8) - FCFuncElement("Exchange16", COMInterlocked::Exchange16) FCFuncElement("Exchange32", COMInterlocked::Exchange32) FCFuncElement("Exchange64", COMInterlocked::Exchange64) FCFuncElement("ExchangeObject", COMInterlocked::ExchangeObject) - FCFuncElement("CompareExchange8", COMInterlocked::CompareExchange8) - FCFuncElement("CompareExchange16", COMInterlocked::CompareExchange16) FCFuncElement("CompareExchange32", COMInterlocked::CompareExchange32) FCFuncElement("CompareExchange64", COMInterlocked::CompareExchange64) FCFuncElement("CompareExchangeObject", COMInterlocked::CompareExchangeObject) diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/Interlocked.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/Interlocked.cs index 31e10ce2b11e75..e6546fc7c0680a 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Threading/Interlocked.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Threading/Interlocked.cs @@ -1,6 +1,7 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System.Diagnostics; using System.Runtime.CompilerServices; namespace System.Threading @@ -67,9 +68,81 @@ public static sbyte Exchange(ref sbyte location1, sbyte value) => /// The address of location1 is a null pointer. [Intrinsic] [MethodImpl(MethodImplOptions.AggressiveInlining)] + public static short Exchange(ref short location1, short value) => + (short)Exchange(ref Unsafe.As(ref location1), (ushort)value); + + /// Sets a 8-bit unsigned integer to a specified value and returns the original value, as an atomic operation. + /// The variable to set to the specified value. + /// The value to which the parameter is set. + /// The original value of . + /// The address of location1 is a null pointer. + [Intrinsic] + [MethodImpl(MethodImplOptions.AggressiveInlining)] + public static unsafe byte Exchange(ref byte location1, byte value) + { +#if TARGET_X86 || TARGET_AMD64 || TARGET_ARM64 + return Exchange(ref location1, value); // Must expand intrinsic +#else + // this relies on GC keeping 4B alignment for refs and on subtracting to such alignment being in the same object + nuint offset = (nuint)Unsafe.AsPointer(ref location1) % sizeof(uint); + ref uint alignedRef = ref Unsafe.As(ref Unsafe.SubtractByteOffset(ref location1, offset)); + int bitOffset = + (int)((BitConverter.IsLittleEndian ? offset : sizeof(uint) - offset - sizeof(byte)) * 8); // to bit offset + Debug.Assert(bitOffset is 0 or 8 or 16 or 24); + uint mask = ~((uint)byte.MaxValue << bitOffset); + uint shiftedValue = (uint)value << bitOffset; + + uint originalValue = 0, newValue; + do + { + // make sure the ref is still aligned + Debug.Assert((nuint)Unsafe.AsPointer(ref alignedRef) % sizeof(uint) == 0); + newValue = originalValue & mask | shiftedValue; + } while (originalValue != + (originalValue = CompareExchange(ref alignedRef, newValue, originalValue))); + + // verify the GC hasn't broken the ref + Debug.Assert((nuint)Unsafe.ByteOffset(ref Unsafe.As(ref alignedRef), ref location1) == offset); + return (byte)(originalValue >> bitOffset); +#endif + } + + /// Sets a 16-bit signed integer to a specified value and returns the original value, as an atomic operation. + /// The variable to set to the specified value. + /// The value to which the parameter is set. + /// The original value of . + /// The address of location1 is a null pointer. + [Intrinsic] + [MethodImpl(MethodImplOptions.AggressiveInlining)] [CLSCompliant(false)] - public static ushort Exchange(ref ushort location1, ushort value) => - (ushort)Exchange(ref Unsafe.As(ref location1), (short)value); + public static unsafe ushort Exchange(ref ushort location1, ushort value) + { +#if TARGET_X86 || TARGET_AMD64 || TARGET_ARM64 + return Exchange(ref location1, value); // Must expand intrinsic +#else + // this relies on GC keeping 4B alignment for refs and on subtracting to such alignment being in the same object + nuint offset = (nuint)Unsafe.AsPointer(ref location1) % sizeof(uint); + ref uint alignedRef = ref Unsafe.As(ref Unsafe.SubtractByteOffset(ref location1, offset)); + int bitOffset = + (int)((BitConverter.IsLittleEndian ? offset : sizeof(uint) - offset - sizeof(byte)) * 8); // to bit offset + Debug.Assert(bitOffset is 0 or 16); + uint mask = ~((uint)ushort.MaxValue << bitOffset); + uint shiftedValue = (uint)value << bitOffset; + + uint originalValue = 0, newValue; + do + { + // make sure the ref is still aligned + Debug.Assert((nuint)Unsafe.AsPointer(ref alignedRef) % sizeof(uint) == 0); + newValue = originalValue & mask | shiftedValue; + } while (originalValue != + (originalValue = CompareExchange(ref alignedRef, newValue, originalValue))); + + // verify the GC hasn't broken the ref + Debug.Assert((nuint)Unsafe.ByteOffset(ref Unsafe.As(ref alignedRef), ref location1) == offset); + return (ushort)(originalValue >> bitOffset); +#endif + } /// Sets a 32-bit unsigned integer to a specified value and returns the original value, as an atomic operation. /// The variable to set to the specified value. @@ -168,9 +241,89 @@ public static sbyte CompareExchange(ref sbyte location1, sbyte value, sbyte comp /// The address of is a null pointer. [Intrinsic] [MethodImpl(MethodImplOptions.AggressiveInlining)] + public static short CompareExchange(ref short location1, short value, short comparand) => + (short)CompareExchange(ref Unsafe.As(ref location1), (ushort)value, (ushort)comparand); + + /// Compares two 8-bit unsigned integers for equality and, if they are equal, replaces the first value. + /// The destination, whose value is compared with and possibly replaced. + /// The value that replaces the destination value if the comparison results in equality. + /// The value that is compared to the value at . + /// The original value in . + /// The address of is a null pointer. + [Intrinsic] + [MethodImpl(MethodImplOptions.AggressiveInlining)] + public static unsafe byte CompareExchange(ref byte location1, byte value, byte comparand) + { +#if TARGET_X86 || TARGET_AMD64 || TARGET_ARM64 + return CompareExchange(ref location1, value, comparand); // Must expand intrinsic +#else + // this relies on GC keeping 4B alignment for refs and on subtracting to such alignment being in the same object + nuint offset = (nuint)Unsafe.AsPointer(ref location1) % sizeof(uint); + ref uint alignedRef = ref Unsafe.As(ref Unsafe.SubtractByteOffset(ref location1, offset)); + int bitOffset = + (int)((BitConverter.IsLittleEndian ? offset : sizeof(uint) - offset - sizeof(byte)) * 8); // to bit offset + Debug.Assert(bitOffset is 0 or 8 or 16 or 24); + uint mask = ~((uint)byte.MaxValue << bitOffset); + uint shiftedValue = (uint)value << bitOffset; + uint shiftedComparand = (uint)comparand << bitOffset; + + uint originalValue, fullComparand, newValue; + do + { + // make sure the ref is still aligned + Debug.Assert((nuint)Unsafe.AsPointer(ref alignedRef) % sizeof(uint) == 0); + originalValue = Volatile.Read(ref alignedRef); + uint otherMemory = originalValue & mask; + fullComparand = otherMemory | shiftedComparand; + newValue = otherMemory | shiftedValue; + } while (originalValue != CompareExchange(ref alignedRef, newValue, fullComparand)); + + // verify the GC hasn't broken the ref + Debug.Assert((nuint)Unsafe.ByteOffset(ref Unsafe.As(ref alignedRef), ref location1) == offset); + return (byte)(originalValue >> bitOffset); +#endif + } + + /// Compares two 16-bit signed integers for equality and, if they are equal, replaces the first value. + /// The destination, whose value is compared with and possibly replaced. + /// The value that replaces the destination value if the comparison results in equality. + /// The value that is compared to the value at . + /// The original value in . + /// The address of is a null pointer. + [Intrinsic] + [MethodImpl(MethodImplOptions.AggressiveInlining)] [CLSCompliant(false)] - public static ushort CompareExchange(ref ushort location1, ushort value, ushort comparand) => - (ushort)CompareExchange(ref Unsafe.As(ref location1), (short)value, (short)comparand); + public static unsafe ushort CompareExchange(ref ushort location1, ushort value, ushort comparand) + { +#if TARGET_X86 || TARGET_AMD64 || TARGET_ARM64 + return CompareExchange(ref location1, value, comparand); // Must expand intrinsic +#else + // this relies on GC keeping 4B alignment for refs and on subtracting to such alignment being in the same object + nuint offset = (nuint)Unsafe.AsPointer(ref location1) % sizeof(uint); + ref uint alignedRef = ref Unsafe.As(ref Unsafe.SubtractByteOffset(ref location1, offset)); + int bitOffset = + (int)((BitConverter.IsLittleEndian ? offset : sizeof(uint) - offset - sizeof(byte)) * 8); // to bit offset + Debug.Assert(bitOffset is 0 or 16); + uint mask = ~((uint)ushort.MaxValue << bitOffset); + uint shiftedValue = (uint)value << bitOffset; + uint shiftedComparand = (uint)comparand << bitOffset; + + uint originalValue, fullComparand, newValue; + do + { + // make sure the ref is still aligned + Debug.Assert((nuint)Unsafe.AsPointer(ref alignedRef) % sizeof(uint) == 0); + originalValue = Volatile.Read(ref alignedRef); + uint otherMemory = originalValue & mask; + fullComparand = otherMemory | shiftedComparand; + newValue = otherMemory | shiftedValue; + } while (originalValue != CompareExchange(ref alignedRef, newValue, fullComparand)); + + // verify the GC hasn't broken the ref + Debug.Assert((nuint)Unsafe.ByteOffset(ref Unsafe.As(ref alignedRef), ref location1) == offset); + return (ushort)(originalValue >> bitOffset); +#endif + } /// Compares two 32-bit unsigned integers for equality and, if they are equal, replaces the first value. /// The destination, whose value is compared with and possibly replaced. diff --git a/src/mono/System.Private.CoreLib/src/System/Threading/Interlocked.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Threading/Interlocked.Mono.cs index 36adaa901e8b04..79e193e70e09c0 100644 --- a/src/mono/System.Private.CoreLib/src/System/Threading/Interlocked.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Threading/Interlocked.Mono.cs @@ -8,14 +8,6 @@ namespace System.Threading { public static partial class Interlocked { - [Intrinsic] - [MethodImplAttribute(MethodImplOptions.InternalCall)] - public static extern byte CompareExchange(ref byte location1, byte value, byte comparand); - - [Intrinsic] - [MethodImplAttribute(MethodImplOptions.InternalCall)] - public static extern short CompareExchange(ref short location1, short value, short comparand); - [Intrinsic] [MethodImplAttribute(MethodImplOptions.InternalCall)] public static extern int CompareExchange(ref int location1, int value, int comparand); @@ -61,14 +53,6 @@ public static partial class Interlocked [MethodImplAttribute(MethodImplOptions.InternalCall)] public static extern long Increment(ref long location); - [Intrinsic] - [MethodImplAttribute(MethodImplOptions.InternalCall)] - public static extern byte Exchange(ref byte location1, byte value); - - [Intrinsic] - [MethodImplAttribute(MethodImplOptions.InternalCall)] - public static extern short Exchange(ref short location1, short value); - [Intrinsic] [MethodImplAttribute(MethodImplOptions.InternalCall)] public static extern int Exchange(ref int location1, int value); diff --git a/src/mono/mono/metadata/icall-def.h b/src/mono/mono/metadata/icall-def.h index 06f3ab888b729d..baf66fab822513 100644 --- a/src/mono/mono/metadata/icall-def.h +++ b/src/mono/mono/metadata/icall-def.h @@ -556,17 +556,13 @@ HANDLES(STRING_11, "InternalIsInterned", ves_icall_System_String_InternalIsInter ICALL_TYPE(ILOCK, "System.Threading.Interlocked", ILOCK_1) NOHANDLES(ICALL(ILOCK_1, "Add(int&,int)", ves_icall_System_Threading_Interlocked_Add_Int)) NOHANDLES(ICALL(ILOCK_2, "Add(long&,long)", ves_icall_System_Threading_Interlocked_Add_Long)) -NOHANDLES(ICALL(ILOCK_24, "CompareExchange(byte&,byte,byte)", ves_icall_System_Threading_Interlocked_CompareExchange_Byte)) NOHANDLES(ICALL(ILOCK_5, "CompareExchange(int&,int,int)", ves_icall_System_Threading_Interlocked_CompareExchange_Int)) NOHANDLES(ICALL(ILOCK_6, "CompareExchange(int&,int,int,bool&)", ves_icall_System_Threading_Interlocked_CompareExchange_Int_Success)) -NOHANDLES(ICALL(ILOCK_25, "CompareExchange(int16&,int16,int16)", ves_icall_System_Threading_Interlocked_CompareExchange_Short)) NOHANDLES(ICALL(ILOCK_8, "CompareExchange(long&,long,long)", ves_icall_System_Threading_Interlocked_CompareExchange_Long)) NOHANDLES(ICALL(ILOCK_9, "CompareExchange(object&,object&,object&,object&)", ves_icall_System_Threading_Interlocked_CompareExchange_Object)) NOHANDLES(ICALL(ILOCK_11, "Decrement(int&)", ves_icall_System_Threading_Interlocked_Decrement_Int)) NOHANDLES(ICALL(ILOCK_12, "Decrement(long&)", ves_icall_System_Threading_Interlocked_Decrement_Long)) -NOHANDLES(ICALL(ILOCK_26, "Exchange(byte&,byte)", ves_icall_System_Threading_Interlocked_Exchange_Byte)) NOHANDLES(ICALL(ILOCK_15, "Exchange(int&,int)", ves_icall_System_Threading_Interlocked_Exchange_Int)) -NOHANDLES(ICALL(ILOCK_27, "Exchange(int16&,int16)", ves_icall_System_Threading_Interlocked_Exchange_Short)) NOHANDLES(ICALL(ILOCK_17, "Exchange(long&,long)", ves_icall_System_Threading_Interlocked_Exchange_Long)) NOHANDLES(ICALL(ILOCK_18, "Exchange(object&,object&,object&)", ves_icall_System_Threading_Interlocked_Exchange_Object)) NOHANDLES(ICALL(ILOCK_20, "Increment(int&)", ves_icall_System_Threading_Interlocked_Increment_Int)) diff --git a/src/mono/mono/metadata/threads-types.h b/src/mono/mono/metadata/threads-types.h index 576cdcb25ca990..c92e61497dba60 100644 --- a/src/mono/mono/metadata/threads-types.h +++ b/src/mono/mono/metadata/threads-types.h @@ -136,12 +136,6 @@ gint32 ves_icall_System_Threading_Interlocked_Decrement_Int(gint32 *location); ICALL_EXPORT gint64 ves_icall_System_Threading_Interlocked_Decrement_Long(gint64 * location); -ICALL_EXPORT -guint8 ves_icall_System_Threading_Interlocked_Exchange_Byte(guint8 *location, guint8 value); - -ICALL_EXPORT -gint16 ves_icall_System_Threading_Interlocked_Exchange_Short(gint16 *location, gint16 value); - ICALL_EXPORT gint32 ves_icall_System_Threading_Interlocked_Exchange_Int(gint32 *location, gint32 value); @@ -151,12 +145,6 @@ gint64 ves_icall_System_Threading_Interlocked_Exchange_Long(gint64 *location, gi ICALL_EXPORT void ves_icall_System_Threading_Interlocked_Exchange_Object (MonoObject *volatile*location, MonoObject *volatile*value, MonoObject *volatile*res); -ICALL_EXPORT -guint8 ves_icall_System_Threading_Interlocked_CompareExchange_Byte(guint8 *location, guint8 value, guint8 comparand); - -ICALL_EXPORT -gint16 ves_icall_System_Threading_Interlocked_CompareExchange_Short(gint16 *location, gint16 value, gint16 comparand); - ICALL_EXPORT gint32 ves_icall_System_Threading_Interlocked_CompareExchange_Int(gint32 *location, gint32 value, gint32 comparand); diff --git a/src/mono/mono/metadata/threads.c b/src/mono/mono/metadata/threads.c index 0fe313db637cf9..38696ef7ca4d09 100644 --- a/src/mono/mono/metadata/threads.c +++ b/src/mono/mono/metadata/threads.c @@ -2146,22 +2146,6 @@ gint64 ves_icall_System_Threading_Interlocked_Decrement_Long (gint64 * location) return mono_atomic_dec_i64 (location); } -guint8 ves_icall_System_Threading_Interlocked_Exchange_Byte (guint8 *location, guint8 value) -{ - if (G_UNLIKELY (!location)) - return (guint8)set_pending_null_reference_exception (); - - return mono_atomic_xchg_u8(location, value); -} - -gint16 ves_icall_System_Threading_Interlocked_Exchange_Short (gint16 *location, gint16 value) -{ - if (G_UNLIKELY (!location)) - return (gint16)set_pending_null_reference_exception (); - - return mono_atomic_xchg_i16(location, value); -} - gint32 ves_icall_System_Threading_Interlocked_Exchange_Int (gint32 *location, gint32 value) { if (G_UNLIKELY (!location)) @@ -2210,22 +2194,6 @@ ves_icall_System_Threading_Interlocked_Exchange_Long (gint64 *location, gint64 v return mono_atomic_xchg_i64 (location, value); } -guint8 ves_icall_System_Threading_Interlocked_CompareExchange_Byte(guint8 *location, guint8 value, guint8 comparand) -{ - if (G_UNLIKELY (!location)) - return (guint8)set_pending_null_reference_exception (); - - return mono_atomic_cas_u8(location, value, comparand); -} - -gint16 ves_icall_System_Threading_Interlocked_CompareExchange_Short(gint16 *location, gint16 value, gint16 comparand) -{ - if (G_UNLIKELY (!location)) - return (gint16)set_pending_null_reference_exception (); - - return mono_atomic_cas_i16(location, value, comparand); -} - gint32 ves_icall_System_Threading_Interlocked_CompareExchange_Int(gint32 *location, gint32 value, gint32 comparand) { if (G_UNLIKELY (!location)) diff --git a/src/mono/mono/utils/atomic.h b/src/mono/mono/utils/atomic.h index 83a835da1895ed..7c7c684ab94eb9 100644 --- a/src/mono/mono/utils/atomic.h +++ b/src/mono/mono/utils/atomic.h @@ -95,22 +95,6 @@ Apple targets have historically being problematic, xcode 4.6 would miscompile th #include -static inline guint8 -mono_atomic_cas_u8 (volatile guint8 *dest, guint8 exch, guint8 comp) -{ - g_static_assert (sizeof (atomic_uchar) == sizeof (*dest) && ATOMIC_CHAR_LOCK_FREE == 2); - (void)atomic_compare_exchange_strong ((volatile atomic_uchar *)dest, &comp, exch); - return comp; -} - -static inline gint16 -mono_atomic_cas_i16 (volatile gint16 *dest, gint16 exch, gint16 comp) -{ - g_static_assert (sizeof (atomic_short) == sizeof (*dest) && ATOMIC_SHORT_LOCK_FREE == 2); - (void)atomic_compare_exchange_strong ((volatile atomic_short *)dest, &comp, exch); - return comp; -} - static inline gint32 mono_atomic_cas_i32 (volatile gint32 *dest, gint32 exch, gint32 comp) { @@ -187,20 +171,6 @@ mono_atomic_dec_i64 (volatile gint64 *dest) return mono_atomic_add_i64 (dest, -1); } -static inline guint8 -mono_atomic_xchg_u8 (volatile guint8 *dest, guint8 exch) -{ - g_static_assert (sizeof (atomic_uchar) == sizeof (*dest) && ATOMIC_CHAR_LOCK_FREE == 2); - return atomic_exchange ((volatile atomic_uchar *)dest, exch); -} - -static inline gint16 -mono_atomic_xchg_i16 (volatile gint16 *dest, gint16 exch) -{ - g_static_assert (sizeof (atomic_short) == sizeof (*dest) && ATOMIC_SHORT_LOCK_FREE == 2); - return atomic_exchange ((volatile atomic_short *)dest, exch); -} - static inline gint32 mono_atomic_xchg_i32 (volatile gint32 *dest, gint32 exch) { @@ -341,18 +311,6 @@ mono_atomic_store_ptr (volatile gpointer *dst, gpointer val) #include #include -static inline guint8 -mono_atomic_cas_u8 (volatile guint8 *dest, guint8 exch, guint8 comp) -{ - return _InterlockedCompareExchange8 ((CHAR volatile *)dest, (CHAR)exch, (CHAR)comp); -} - -static inline gint16 -mono_atomic_cas_i16 (volatile gint16 *dest, gint16 exch, gint16 comp) -{ - return _InterlockedCompareExchange16 ((SHORT volatile *)dest, (SHORT)exch, (SHORT)comp); -} - static inline gint32 mono_atomic_cas_i32 (volatile gint32 *dest, gint32 exch, gint32 comp) { @@ -407,18 +365,6 @@ mono_atomic_dec_i64 (volatile gint64 *dest) return InterlockedDecrement64 ((LONG64 volatile *)dest); } -static inline guint8 -mono_atomic_xchg_u8 (volatile guint8 *dest, guint8 exch) -{ - return _InterlockedExchange8 ((CHAR volatile *)dest, (CHAR)exch); -} - -static inline gint16 -mono_atomic_xchg_i16 (volatile gint16 *dest, gint16 exch) -{ - return _InterlockedExchange16 ((SHORT volatile *)dest, (SHORT)exch); -} - static inline gint32 mono_atomic_xchg_i32 (volatile gint32 *dest, gint32 exch) { @@ -562,18 +508,6 @@ mono_atomic_store_ptr (volatile gpointer *dst, gpointer val) #define gcc_sync_fetch_and_add(a, b) __sync_fetch_and_add (a, b) #endif -static inline guint8 mono_atomic_cas_u8(volatile guint8 *dest, - guint8 exch, guint8 comp) -{ - return gcc_sync_val_compare_and_swap (dest, comp, exch); -} - -static inline gint16 mono_atomic_cas_i16(volatile gint16 *dest, - gint16 exch, gint16 comp) -{ - return gcc_sync_val_compare_and_swap (dest, comp, exch); -} - static inline gint32 mono_atomic_cas_i32(volatile gint32 *dest, gint32 exch, gint32 comp) { @@ -600,24 +534,6 @@ static inline gint32 mono_atomic_dec_i32(volatile gint32 *val) return gcc_sync_sub_and_fetch (val, 1); } -static inline guint8 mono_atomic_xchg_u8(volatile guint8 *val, guint8 new_val) -{ - guint8 old_val; - do { - old_val = *val; - } while (gcc_sync_val_compare_and_swap (val, old_val, new_val) != old_val); - return old_val; -} - -static inline gint16 mono_atomic_xchg_i16(volatile gint16 *val, gint16 new_val) -{ - gint16 old_val; - do { - old_val = *val; - } while (gcc_sync_val_compare_and_swap (val, old_val, new_val) != old_val); - return old_val; -} - static inline gint32 mono_atomic_xchg_i32(volatile gint32 *val, gint32 new_val) { gint32 old_val; @@ -809,12 +725,7 @@ static inline void mono_atomic_store_i64(volatile gint64 *dst, gint64 val) #define WAPI_NO_ATOMIC_ASM -/* Fallbacks seem to not be used anymore, they should be removed - * or small type ones should be added in case we find a platform that still needs them. - * extern guint8 mono_atomic_cas_u8(volatile guint8 *dest, guint8 exch, guint8 comp); - * extern gint16 mono_atomic_cas_i16(volatile gint16 *dest, gint16 exch, gint16 comp); - * extern guint8 mono_atomic_xchg_u8(volatile guint8 *dest, guint8 exch); - * extern gint16 mono_atomic_xchg_i16(volatile gint16 *dest, gint16 exch); */ +/* Fallbacks seem to not be used anymore, they should be removed. */ extern gint32 mono_atomic_cas_i32(volatile gint32 *dest, gint32 exch, gint32 comp); extern gint64 mono_atomic_cas_i64(volatile gint64 *dest, gint64 exch, gint64 comp); extern gpointer mono_atomic_cas_ptr(volatile gpointer *dest, gpointer exch, gpointer comp); diff --git a/src/tests/JIT/Intrinsics/Interlocked.cs b/src/tests/JIT/Intrinsics/Interlocked.cs index 5ac15785448e63..0091933666cd62 100644 --- a/src/tests/JIT/Intrinsics/Interlocked.cs +++ b/src/tests/JIT/Intrinsics/Interlocked.cs @@ -2,8 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. // -using System; -using System.Threading; +using System.Diagnostics; using System.Runtime.CompilerServices; using System.Runtime.InteropServices; using Xunit; @@ -12,7 +11,50 @@ namespace InterlockedTest { public unsafe class Program { - private static int _errors = 0; + [StructLayout(LayoutKind.Explicit)] + private sealed class Box + { + [FieldOffset(0)] + private long memory; + [FieldOffset(8)] + private long val; + [FieldOffset(16)] + public nuint offset; + + public long Memory => memory; + + [MethodImpl(MethodImplOptions.NoInlining)] + public ref T GetRef() where T : unmanaged + { + return ref Unsafe.Add(ref Unsafe.As(ref memory), offset); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + public long GetValue(T value, [CallerLineNumber] int line = 0) where T : unmanaged + { + long l = val; + if (l is not (0L or -1L)) + { + Console.WriteLine($"Line {line}: found write out of bounds at offset {offset}"); + _errors++; + } + Unsafe.Add(ref Unsafe.As(ref l), offset) = value; + return l; + } + + public void Set(long value, [CallerLineNumber] int line = 0) + { + if (value != ~val) + { + Console.WriteLine($"Line {line}: found corrupt check value at offset {offset}"); + _errors++; + } + memory = val = value; + } + } + + private static int _errors; + private static Box _box; [Fact] public static int TestEntryPoint() @@ -35,144 +77,135 @@ public static int TestEntryPoint() [MethodImpl(MethodImplOptions.NoInlining)] static delegate* CompareExchangeUShort() => &Interlocked.CompareExchange; - long mem = -1; - [MethodImpl(MethodImplOptions.NoInlining)] - long GetValue(T val) where T : unmanaged + _box = new(); + for (; _box.offset < sizeof(long) / sizeof(ushort); _box.offset++) { - Unsafe.As(ref mem) = val; - return mem; - } + _box.Set(-1); + Equals(255, Interlocked.Exchange(ref _box.GetRef(), 254)); + Equals(_box.GetValue(254), _box.Memory); + Equals(254, ExchangeByte()(ref _box.GetRef(), 253)); + Equals(_box.GetValue(253), _box.Memory); + + _box.Set(0); + Equals(0, Interlocked.Exchange(ref _box.GetRef(), -4)); + Equals(_box.GetValue(-4), _box.Memory); + Equals(-4, ExchangeSByte()(ref _box.GetRef(), -5)); + Equals(_box.GetValue(-5), _box.Memory); - long l = -1; - Equals(255, Interlocked.Exchange(ref Unsafe.As(ref l), 254)); - Equals(GetValue(254), l); - Equals(254, ExchangeByte()(ref Unsafe.As(ref l), 253)); - Equals(GetValue(253), l); - - mem = 0; - l = 0; - Equals(0, Interlocked.Exchange(ref Unsafe.As(ref l), -4)); - Equals(GetValue(-4), l); - Equals(-4, ExchangeSByte()(ref Unsafe.As(ref l), -5)); - Equals(GetValue(-5), l); - - mem = -1; - l = -1; - Equals(255, Interlocked.CompareExchange(ref Unsafe.As(ref l), 254, 255)); - Equals(GetValue(254), l); - Equals(254, CompareExchangeByte()(ref Unsafe.As(ref l), 253, 254)); - Equals(GetValue(253), l); - - mem = 0; - l = 0; - Equals(0, Interlocked.CompareExchange(ref Unsafe.As(ref l), -4, 0)); - Equals(GetValue(-4), l); - Equals(-4, CompareExchangeSByte()(ref Unsafe.As(ref l), -5, -4)); - Equals(GetValue(-5), l); - - Equals(251, Interlocked.CompareExchange(ref Unsafe.As(ref l), 2, 10)); - Equals(GetValue(251), l); - Equals(251, CompareExchangeByte()(ref Unsafe.As(ref l), 2, 10)); - Equals(GetValue(251), l); - Equals(-5, Interlocked.CompareExchange(ref Unsafe.As(ref l), 2, 10)); - Equals(GetValue(-5), l); - Equals(-5, CompareExchangeSByte()(ref Unsafe.As(ref l), 2, 10)); - Equals(GetValue(-5), l); - - mem = 0; - l = 0; - Equals(0, Interlocked.Exchange(ref Unsafe.As(ref l), -2)); - Equals(GetValue(-2), l); - Equals(-2, ExchangeShort()(ref Unsafe.As(ref l), -3)); - Equals(GetValue(-3), l); - - mem = -1; - l = -1; - Equals(65535, Interlocked.Exchange(ref Unsafe.As(ref l), 65532)); - Equals(GetValue(65532), l); - Equals(65532, ExchangeUShort()(ref Unsafe.As(ref l), 65531)); - Equals(GetValue(65531), l); - - mem = 0; - l = 0; - Equals(0, Interlocked.CompareExchange(ref Unsafe.As(ref l), -2, 0)); - Equals(GetValue(-2), l); - Equals(-2, CompareExchangeShort()(ref Unsafe.As(ref l), -3, -2)); - Equals(GetValue(-3), l); - - mem = -1; - l = -1; - Equals(65535, Interlocked.CompareExchange(ref Unsafe.As(ref l), 65532, 65535)); - Equals(GetValue(65532), l); - Equals(65532, CompareExchangeUShort()(ref Unsafe.As(ref l), 65531, 65532)); - Equals(GetValue(65531), l); - - Equals(-5, Interlocked.CompareExchange(ref Unsafe.As(ref l), 1444, 1555)); - Equals(GetValue(-5), l); - Equals(-5, CompareExchangeShort()(ref Unsafe.As(ref l), 1444, 1555)); - Equals(GetValue(-5), l); - Equals(65531, Interlocked.CompareExchange(ref Unsafe.As(ref l), 1444, 1555)); - Equals(GetValue(65531), l); - Equals(65531, CompareExchangeUShort()(ref Unsafe.As(ref l), 1444, 1555)); - Equals(GetValue(65531), l); - - mem = -1; - l = -1; - Interlocked.Exchange(ref Unsafe.As(ref l), 123); - Equals(GetValue(123), l); - ExchangeByte()(ref Unsafe.As(ref l), 124); - Equals(GetValue(124), l); - Interlocked.Exchange(ref Unsafe.As(ref l), 125); - Equals(GetValue(125), l); - ExchangeSByte()(ref Unsafe.As(ref l), 126); - Equals(GetValue(126), l); - - Interlocked.CompareExchange(ref Unsafe.As(ref l), 55, 126); - Equals(GetValue(55), l); - CompareExchangeByte()(ref Unsafe.As(ref l), 56, 55); - Equals(GetValue(56), l); - Interlocked.CompareExchange(ref Unsafe.As(ref l), 57, 56); - Equals(GetValue(57), l); - CompareExchangeSByte()(ref Unsafe.As(ref l), 58, 57); - Equals(GetValue(58), l); - - Interlocked.CompareExchange(ref Unsafe.As(ref l), 10, 2); - Equals(GetValue(58), l); - CompareExchangeByte()(ref Unsafe.As(ref l), 10, 2); - Equals(GetValue(58), l); - Interlocked.CompareExchange(ref Unsafe.As(ref l), 10, 2); - Equals(GetValue(58), l); - CompareExchangeSByte()(ref Unsafe.As(ref l), 10, 2); - Equals(GetValue(58), l); - - mem = -1; - l = -1; - Interlocked.Exchange(ref Unsafe.As(ref l), 12345); - Equals(GetValue(12345), l); - ExchangeShort()(ref Unsafe.As(ref l), 12346); - Equals(GetValue(12346), l); - Interlocked.Exchange(ref Unsafe.As(ref l), 12347); - Equals(GetValue(12347), l); - ExchangeUShort()(ref Unsafe.As(ref l), 12348); - Equals(GetValue(12348), l); - - Interlocked.CompareExchange(ref Unsafe.As(ref l), 1234, 12348); - Equals(GetValue(1234), l); - CompareExchangeShort()(ref Unsafe.As(ref l), 1235, 1234); - Equals(GetValue(1235), l); - Interlocked.CompareExchange(ref Unsafe.As(ref l), 1236, 1235); - Equals(GetValue(1236), l); - CompareExchangeUShort()(ref Unsafe.As(ref l), 1237, 1236); - Equals(GetValue(1237), l); - - Interlocked.CompareExchange(ref Unsafe.As(ref l), 1555, 1444); - Equals(GetValue(1237), l); - CompareExchangeShort()(ref Unsafe.As(ref l), 1555, 1444); - Equals(GetValue(1237), l); - Interlocked.CompareExchange(ref Unsafe.As(ref l), 1555, 1444); - Equals(GetValue(1237), l); - CompareExchangeUShort()(ref Unsafe.As(ref l), 1555, 1444); - Equals(GetValue(1237), l); + _box.Set(-1); + Equals(255, Interlocked.CompareExchange(ref _box.GetRef(), 254, 255)); + Equals(_box.GetValue(254), _box.Memory); + Equals(254, CompareExchangeByte()(ref _box.GetRef(), 253, 254)); + Equals(_box.GetValue(253), _box.Memory); + + _box.Set(0); + Equals(0, Interlocked.CompareExchange(ref _box.GetRef(), -4, 0)); + Equals(_box.GetValue(-4), _box.Memory); + Equals(-4, CompareExchangeSByte()(ref _box.GetRef(), -5, -4)); + Equals(_box.GetValue(-5), _box.Memory); + + Equals(251, Interlocked.CompareExchange(ref _box.GetRef(), 2, 10)); + Equals(_box.GetValue(251), _box.Memory); + Equals(251, CompareExchangeByte()(ref _box.GetRef(), 2, 10)); + Equals(_box.GetValue(251), _box.Memory); + Equals(-5, Interlocked.CompareExchange(ref _box.GetRef(), 2, 10)); + Equals(_box.GetValue(-5), _box.Memory); + Equals(-5, CompareExchangeSByte()(ref _box.GetRef(), 2, 10)); + Equals(_box.GetValue(-5), _box.Memory); + + _box.Set(-1); + _box.Set(0); + Equals(0, Interlocked.Exchange(ref _box.GetRef(), -2)); + Equals(_box.GetValue(-2), _box.Memory); + Equals(-2, ExchangeShort()(ref _box.GetRef(), -3)); + Equals(_box.GetValue(-3), _box.Memory); + + _box.Set(-1); + Equals(65535, Interlocked.Exchange(ref _box.GetRef(), 65532)); + Equals(_box.GetValue(65532), _box.Memory); + Equals(65532, ExchangeUShort()(ref _box.GetRef(), 65531)); + Equals(_box.GetValue(65531), _box.Memory); + + _box.Set(0); + Equals(0, Interlocked.CompareExchange(ref _box.GetRef(), -2, 0)); + Equals(_box.GetValue(-2), _box.Memory); + Equals(-2, CompareExchangeShort()(ref _box.GetRef(), -3, -2)); + Equals(_box.GetValue(-3), _box.Memory); + + _box.Set(-1); + Equals(65535, Interlocked.CompareExchange(ref _box.GetRef(), 65532, 65535)); + Equals(_box.GetValue(65532), _box.Memory); + Equals(65532, CompareExchangeUShort()(ref _box.GetRef(), 65531, 65532)); + Equals(_box.GetValue(65531), _box.Memory); + + Equals(-5, Interlocked.CompareExchange(ref _box.GetRef(), 1444, 1555)); + Equals(_box.GetValue(-5), _box.Memory); + Equals(-5, CompareExchangeShort()(ref _box.GetRef(), 1444, 1555)); + Equals(_box.GetValue(-5), _box.Memory); + Equals(65531, Interlocked.CompareExchange(ref _box.GetRef(), 1444, 1555)); + Equals(_box.GetValue(65531), _box.Memory); + Equals(65531, CompareExchangeUShort()(ref _box.GetRef(), 1444, 1555)); + Equals(_box.GetValue(65531), _box.Memory); + + _box.Set(0); + _box.Set(-1); + Interlocked.Exchange(ref _box.GetRef(), 123); + Equals(_box.GetValue(123), _box.Memory); + ExchangeByte()(ref _box.GetRef(), 124); + Equals(_box.GetValue(124), _box.Memory); + Interlocked.Exchange(ref _box.GetRef(), 125); + Equals(_box.GetValue(125), _box.Memory); + ExchangeSByte()(ref _box.GetRef(), 126); + Equals(_box.GetValue(126), _box.Memory); + + Interlocked.CompareExchange(ref _box.GetRef(), 55, 126); + Equals(_box.GetValue(55), _box.Memory); + CompareExchangeByte()(ref _box.GetRef(), 56, 55); + Equals(_box.GetValue(56), _box.Memory); + Interlocked.CompareExchange(ref _box.GetRef(), 57, 56); + Equals(_box.GetValue(57), _box.Memory); + CompareExchangeSByte()(ref _box.GetRef(), 58, 57); + Equals(_box.GetValue(58), _box.Memory); + + Interlocked.CompareExchange(ref _box.GetRef(), 10, 2); + Equals(_box.GetValue(58), _box.Memory); + CompareExchangeByte()(ref _box.GetRef(), 10, 2); + Equals(_box.GetValue(58), _box.Memory); + Interlocked.CompareExchange(ref _box.GetRef(), 10, 2); + Equals(_box.GetValue(58), _box.Memory); + CompareExchangeSByte()(ref _box.GetRef(), 10, 2); + Equals(_box.GetValue(58), _box.Memory); + + _box.Set(0); + _box.Set(-1); + Interlocked.Exchange(ref _box.GetRef(), 12345); + Equals(_box.GetValue(12345), _box.Memory); + ExchangeShort()(ref _box.GetRef(), 12346); + Equals(_box.GetValue(12346), _box.Memory); + Interlocked.Exchange(ref _box.GetRef(), 12347); + Equals(_box.GetValue(12347), _box.Memory); + ExchangeUShort()(ref _box.GetRef(), 12348); + Equals(_box.GetValue(12348), _box.Memory); + + Interlocked.CompareExchange(ref _box.GetRef(), 1234, 12348); + Equals(_box.GetValue(1234), _box.Memory); + CompareExchangeShort()(ref _box.GetRef(), 1235, 1234); + Equals(_box.GetValue(1235), _box.Memory); + Interlocked.CompareExchange(ref _box.GetRef(), 1236, 1235); + Equals(_box.GetValue(1236), _box.Memory); + CompareExchangeUShort()(ref _box.GetRef(), 1237, 1236); + Equals(_box.GetValue(1237), _box.Memory); + + Interlocked.CompareExchange(ref _box.GetRef(), 1555, 1444); + Equals(_box.GetValue(1237), _box.Memory); + CompareExchangeShort()(ref _box.GetRef(), 1555, 1444); + Equals(_box.GetValue(1237), _box.Memory); + Interlocked.CompareExchange(ref _box.GetRef(), 1555, 1444); + Equals(_box.GetValue(1237), _box.Memory); + CompareExchangeUShort()(ref _box.GetRef(), 1555, 1444); + Equals(_box.GetValue(1237), _box.Memory); + _box.Set(0); + } ThrowsNRE(() => { Interlocked.Exchange(ref Unsafe.NullRef(), 0); }); ThrowsNRE(() => { Interlocked.Exchange(ref Unsafe.NullRef(), 0); }); @@ -195,17 +228,16 @@ long GetValue(T val) where T : unmanaged } [MethodImpl(MethodImplOptions.NoInlining)] - static void Equals(long left, long right, [CallerLineNumber] int line = 0, [CallerFilePath] string file = "") + private static void Equals(long left, long right, [CallerLineNumber] int line = 0, [CallerFilePath] string file = "") { - if (left != right) - { - Console.WriteLine($"{file}:L{line} test failed (expected: equal, actual: {left}-{right})."); - _errors++; - } + if (left == right) + return; + Console.WriteLine($"{file}:L{line} test failed (not equal, expected: {left}, actual: {right}) at offset {_box.offset}."); + _errors++; } [MethodImpl(MethodImplOptions.NoInlining)] - static void ThrowsNRE(Action action, [CallerLineNumber] int line = 0, [CallerFilePath] string file = "") + private static void ThrowsNRE(Action action, [CallerLineNumber] int line = 0, [CallerFilePath] string file = "") { try { From de5d32e1b28bf9a4fe377b88ee37b7c51cbd6376 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Petryka?= <35800402+MichalPetryka@users.noreply.github.com> Date: Tue, 27 Feb 2024 21:01:25 +0100 Subject: [PATCH 2/8] Update Interlocked.cs --- src/tests/JIT/Intrinsics/Interlocked.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/tests/JIT/Intrinsics/Interlocked.cs b/src/tests/JIT/Intrinsics/Interlocked.cs index 0091933666cd62..f344417fd08734 100644 --- a/src/tests/JIT/Intrinsics/Interlocked.cs +++ b/src/tests/JIT/Intrinsics/Interlocked.cs @@ -2,9 +2,11 @@ // The .NET Foundation licenses this file to you under the MIT license. // +using System; using System.Diagnostics; using System.Runtime.CompilerServices; using System.Runtime.InteropServices; +using System.Threading; using Xunit; namespace InterlockedTest From af7699c2678c7da7fd7f6e3e2738f1545d807094 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Petryka?= <35800402+MichalPetryka@users.noreply.github.com> Date: Wed, 28 Feb 2024 01:41:51 +0100 Subject: [PATCH 3/8] Fix Mono --- .../src/System/Threading/Interlocked.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/Interlocked.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/Interlocked.cs index e6546fc7c0680a..0da994dccaa047 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Threading/Interlocked.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Threading/Interlocked.cs @@ -80,7 +80,7 @@ public static short Exchange(ref short location1, short value) => [MethodImpl(MethodImplOptions.AggressiveInlining)] public static unsafe byte Exchange(ref byte location1, byte value) { -#if TARGET_X86 || TARGET_AMD64 || TARGET_ARM64 +#if !MONO && (TARGET_X86 || TARGET_AMD64 || TARGET_ARM64) return Exchange(ref location1, value); // Must expand intrinsic #else // this relies on GC keeping 4B alignment for refs and on subtracting to such alignment being in the same object @@ -117,7 +117,7 @@ public static unsafe byte Exchange(ref byte location1, byte value) [CLSCompliant(false)] public static unsafe ushort Exchange(ref ushort location1, ushort value) { -#if TARGET_X86 || TARGET_AMD64 || TARGET_ARM64 +#if !MONO && (TARGET_X86 || TARGET_AMD64 || TARGET_ARM64) return Exchange(ref location1, value); // Must expand intrinsic #else // this relies on GC keeping 4B alignment for refs and on subtracting to such alignment being in the same object @@ -254,7 +254,7 @@ public static short CompareExchange(ref short location1, short value, short comp [MethodImpl(MethodImplOptions.AggressiveInlining)] public static unsafe byte CompareExchange(ref byte location1, byte value, byte comparand) { -#if TARGET_X86 || TARGET_AMD64 || TARGET_ARM64 +#if !MONO && (TARGET_X86 || TARGET_AMD64 || TARGET_ARM64) return CompareExchange(ref location1, value, comparand); // Must expand intrinsic #else // this relies on GC keeping 4B alignment for refs and on subtracting to such alignment being in the same object @@ -295,7 +295,7 @@ public static unsafe byte CompareExchange(ref byte location1, byte value, byte c [CLSCompliant(false)] public static unsafe ushort CompareExchange(ref ushort location1, ushort value, ushort comparand) { -#if TARGET_X86 || TARGET_AMD64 || TARGET_ARM64 +#if !MONO && (TARGET_X86 || TARGET_AMD64 || TARGET_ARM64) return CompareExchange(ref location1, value, comparand); // Must expand intrinsic #else // this relies on GC keeping 4B alignment for refs and on subtracting to such alignment being in the same object From 6a03a7ca6c45b8a0af85b9a84ffbc443bf5657f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Petryka?= <35800402+MichalPetryka@users.noreply.github.com> Date: Wed, 28 Feb 2024 04:06:31 +0100 Subject: [PATCH 4/8] Improve CompareExchange performance --- .../src/System/Threading/Interlocked.cs | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/Interlocked.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/Interlocked.cs index 0da994dccaa047..a68e82441cf5e8 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Threading/Interlocked.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Threading/Interlocked.cs @@ -267,16 +267,17 @@ public static unsafe byte CompareExchange(ref byte location1, byte value, byte c uint shiftedValue = (uint)value << bitOffset; uint shiftedComparand = (uint)comparand << bitOffset; - uint originalValue, fullComparand, newValue; + uint originalValue = Volatile.Read(ref alignedRef); + uint fullComparand, newValue; do { // make sure the ref is still aligned Debug.Assert((nuint)Unsafe.AsPointer(ref alignedRef) % sizeof(uint) == 0); - originalValue = Volatile.Read(ref alignedRef); uint otherMemory = originalValue & mask; fullComparand = otherMemory | shiftedComparand; newValue = otherMemory | shiftedValue; - } while (originalValue != CompareExchange(ref alignedRef, newValue, fullComparand)); + } while (originalValue != + (originalValue = CompareExchange(ref alignedRef, newValue, fullComparand))); // verify the GC hasn't broken the ref Debug.Assert((nuint)Unsafe.ByteOffset(ref Unsafe.As(ref alignedRef), ref location1) == offset); @@ -308,16 +309,17 @@ public static unsafe ushort CompareExchange(ref ushort location1, ushort value, uint shiftedValue = (uint)value << bitOffset; uint shiftedComparand = (uint)comparand << bitOffset; - uint originalValue, fullComparand, newValue; + uint originalValue = Volatile.Read(ref alignedRef); + uint fullComparand, newValue; do { // make sure the ref is still aligned Debug.Assert((nuint)Unsafe.AsPointer(ref alignedRef) % sizeof(uint) == 0); - originalValue = Volatile.Read(ref alignedRef); uint otherMemory = originalValue & mask; fullComparand = otherMemory | shiftedComparand; newValue = otherMemory | shiftedValue; - } while (originalValue != CompareExchange(ref alignedRef, newValue, fullComparand)); + } while (originalValue != + (originalValue = CompareExchange(ref alignedRef, newValue, fullComparand))); // verify the GC hasn't broken the ref Debug.Assert((nuint)Unsafe.ByteOffset(ref Unsafe.As(ref alignedRef), ref location1) == offset); From 010854f9e676c7dfd65fa5713e6f4a15808373e6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Petryka?= <35800402+MichalPetryka@users.noreply.github.com> Date: Wed, 28 Feb 2024 12:47:45 +0100 Subject: [PATCH 5/8] Optimize for non-zero padding --- .../src/System/Threading/Interlocked.cs | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/Interlocked.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/Interlocked.cs index a68e82441cf5e8..363ede254f8d7e 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Threading/Interlocked.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Threading/Interlocked.cs @@ -92,7 +92,9 @@ public static unsafe byte Exchange(ref byte location1, byte value) uint mask = ~((uint)byte.MaxValue << bitOffset); uint shiftedValue = (uint)value << bitOffset; - uint originalValue = 0, newValue; + // this doesn't need to be volatile since CompareExchange will update stale values + uint originalValue = alignedRef; + uint newValue; do { // make sure the ref is still aligned @@ -129,7 +131,9 @@ public static unsafe ushort Exchange(ref ushort location1, ushort value) uint mask = ~((uint)ushort.MaxValue << bitOffset); uint shiftedValue = (uint)value << bitOffset; - uint originalValue = 0, newValue; + // this doesn't need to be volatile since CompareExchange will update stale values + uint originalValue = alignedRef; + uint newValue; do { // make sure the ref is still aligned @@ -267,7 +271,8 @@ public static unsafe byte CompareExchange(ref byte location1, byte value, byte c uint shiftedValue = (uint)value << bitOffset; uint shiftedComparand = (uint)comparand << bitOffset; - uint originalValue = Volatile.Read(ref alignedRef); + // this doesn't need to be volatile since CompareExchange will update stale values + uint originalValue = alignedRef; uint fullComparand, newValue; do { @@ -309,7 +314,8 @@ public static unsafe ushort CompareExchange(ref ushort location1, ushort value, uint shiftedValue = (uint)value << bitOffset; uint shiftedComparand = (uint)comparand << bitOffset; - uint originalValue = Volatile.Read(ref alignedRef); + // this doesn't need to be volatile since CompareExchange will update stale values + uint originalValue = alignedRef; uint fullComparand, newValue; do { From 565206c600f90d715e20c341686b4d5f407f07a2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Petryka?= <35800402+MichalPetryka@users.noreply.github.com> Date: Sat, 2 Mar 2024 15:38:11 +0100 Subject: [PATCH 6/8] Add static tests --- src/tests/JIT/Intrinsics/Interlocked.cs | 44 +++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/src/tests/JIT/Intrinsics/Interlocked.cs b/src/tests/JIT/Intrinsics/Interlocked.cs index f344417fd08734..09e37223dd9660 100644 --- a/src/tests/JIT/Intrinsics/Interlocked.cs +++ b/src/tests/JIT/Intrinsics/Interlocked.cs @@ -57,6 +57,7 @@ public void Set(long value, [CallerLineNumber] int line = 0) private static int _errors; private static Box _box; + private static uint _staticMemory; [Fact] public static int TestEntryPoint() @@ -226,6 +227,49 @@ public static int TestEntryPoint() ThrowsNRE(() => { CompareExchangeShort()(ref Unsafe.NullRef(), 0, 0); }); ThrowsNRE(() => { CompareExchangeUShort()(ref Unsafe.NullRef(), 0, 0); }); + // test for asserts with statics since their addresses are constant which caused issues earlier + // test with 4B alignment provided by the uint field + _staticMemory = 0; + Equals(0, Interlocked.Exchange(ref Unsafe.As(ref _staticMemory), 255)); + Equals(255, Unsafe.As(ref _staticMemory)); + + _staticMemory = 0; + Equals(0, Interlocked.Exchange(ref Unsafe.As(ref _staticMemory), 65535)); + Equals(65535, Unsafe.As(ref _staticMemory)); + + _staticMemory = 0; + Equals(0, Interlocked.CompareExchange(ref Unsafe.As(ref _staticMemory), 255, 0)); + Equals(255, Unsafe.As(ref _staticMemory)); + Equals(255, Interlocked.CompareExchange(ref Unsafe.As(ref _staticMemory), 1, 0)); + Equals(255, Unsafe.As(ref _staticMemory)); + + _staticMemory = 0; + Equals(0, Interlocked.CompareExchange(ref Unsafe.As(ref _staticMemory), 65535, 0)); + Equals(65535, Unsafe.As(ref _staticMemory)); + Equals(65535, Interlocked.CompareExchange(ref Unsafe.As(ref _staticMemory), 1, 0)); + Equals(65535, Unsafe.As(ref _staticMemory)); + + // offset the address by 1 to avoid 4B alignment + _staticMemory = 0; + Equals(0, Interlocked.Exchange(ref Unsafe.Add(ref Unsafe.As(ref _staticMemory), 1), 255)); + Equals(255, Unsafe.Add(ref Unsafe.As(ref _staticMemory), 1)); + + _staticMemory = 0; + Equals(0, Interlocked.Exchange(ref Unsafe.Add(ref Unsafe.As(ref _staticMemory), 1), 65535)); + Equals(65535, Unsafe.Add(ref Unsafe.As(ref _staticMemory), 1)); + + _staticMemory = 0; + Equals(0, Interlocked.CompareExchange(ref Unsafe.Add(ref Unsafe.As(ref _staticMemory), 1), 255, 0)); + Equals(255, Unsafe.Add(ref Unsafe.As(ref _staticMemory), 1)); + Equals(255, Interlocked.CompareExchange(ref Unsafe.Add(ref Unsafe.As(ref _staticMemory), 1), 1, 0)); + Equals(255, Unsafe.Add(ref Unsafe.As(ref _staticMemory), 1)); + + _staticMemory = 0; + Equals(0, Interlocked.CompareExchange(ref Unsafe.Add(ref Unsafe.As(ref _staticMemory), 1), 65535, 0)); + Equals(65535, Unsafe.Add(ref Unsafe.As(ref _staticMemory), 1)); + Equals(65535, Interlocked.CompareExchange(ref Unsafe.Add(ref Unsafe.As(ref _staticMemory), 1), 1, 0)); + Equals(65535, Unsafe.Add(ref Unsafe.As(ref _staticMemory), 1)); + return 100 + _errors; } From a2d19d8ac5af8a0e133b86e56c9d4edbccf146fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Petryka?= <35800402+MichalPetryka@users.noreply.github.com> Date: Thu, 21 Mar 2024 18:47:02 +0100 Subject: [PATCH 7/8] Remove AsPointer Co-authored-by: Hamish Arblaster --- .../src/System/Threading/Interlocked.cs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/Interlocked.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/Interlocked.cs index 363ede254f8d7e..18e9f86cdb7cdc 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Threading/Interlocked.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Threading/Interlocked.cs @@ -84,7 +84,7 @@ public static unsafe byte Exchange(ref byte location1, byte value) return Exchange(ref location1, value); // Must expand intrinsic #else // this relies on GC keeping 4B alignment for refs and on subtracting to such alignment being in the same object - nuint offset = (nuint)Unsafe.AsPointer(ref location1) % sizeof(uint); + nuint offset = Unsafe.OpportunisticMisalignment(ref location1, sizeof(uint)); ref uint alignedRef = ref Unsafe.As(ref Unsafe.SubtractByteOffset(ref location1, offset)); int bitOffset = (int)((BitConverter.IsLittleEndian ? offset : sizeof(uint) - offset - sizeof(byte)) * 8); // to bit offset @@ -98,7 +98,7 @@ public static unsafe byte Exchange(ref byte location1, byte value) do { // make sure the ref is still aligned - Debug.Assert((nuint)Unsafe.AsPointer(ref alignedRef) % sizeof(uint) == 0); + Debug.Assert(Unsafe.IsOpportunisticallyAligned(ref alignedRef, sizeof(uint)); newValue = originalValue & mask | shiftedValue; } while (originalValue != (originalValue = CompareExchange(ref alignedRef, newValue, originalValue))); @@ -123,7 +123,7 @@ public static unsafe ushort Exchange(ref ushort location1, ushort value) return Exchange(ref location1, value); // Must expand intrinsic #else // this relies on GC keeping 4B alignment for refs and on subtracting to such alignment being in the same object - nuint offset = (nuint)Unsafe.AsPointer(ref location1) % sizeof(uint); + nuint offset = Unsafe.OpportunisticMisalignment(ref location1, sizeof(uint)); ref uint alignedRef = ref Unsafe.As(ref Unsafe.SubtractByteOffset(ref location1, offset)); int bitOffset = (int)((BitConverter.IsLittleEndian ? offset : sizeof(uint) - offset - sizeof(byte)) * 8); // to bit offset @@ -137,7 +137,7 @@ public static unsafe ushort Exchange(ref ushort location1, ushort value) do { // make sure the ref is still aligned - Debug.Assert((nuint)Unsafe.AsPointer(ref alignedRef) % sizeof(uint) == 0); + Debug.Assert(Unsafe.IsOpportunisticallyAligned(ref alignedRef, sizeof(uint)); newValue = originalValue & mask | shiftedValue; } while (originalValue != (originalValue = CompareExchange(ref alignedRef, newValue, originalValue))); @@ -262,7 +262,7 @@ public static unsafe byte CompareExchange(ref byte location1, byte value, byte c return CompareExchange(ref location1, value, comparand); // Must expand intrinsic #else // this relies on GC keeping 4B alignment for refs and on subtracting to such alignment being in the same object - nuint offset = (nuint)Unsafe.AsPointer(ref location1) % sizeof(uint); + nuint offset = Unsafe.OpportunisticMisalignment(ref location1, sizeof(uint)); ref uint alignedRef = ref Unsafe.As(ref Unsafe.SubtractByteOffset(ref location1, offset)); int bitOffset = (int)((BitConverter.IsLittleEndian ? offset : sizeof(uint) - offset - sizeof(byte)) * 8); // to bit offset @@ -277,7 +277,7 @@ public static unsafe byte CompareExchange(ref byte location1, byte value, byte c do { // make sure the ref is still aligned - Debug.Assert((nuint)Unsafe.AsPointer(ref alignedRef) % sizeof(uint) == 0); + Debug.Assert(Unsafe.IsOpportunisticallyAligned(ref alignedRef, sizeof(uint)); uint otherMemory = originalValue & mask; fullComparand = otherMemory | shiftedComparand; newValue = otherMemory | shiftedValue; @@ -305,7 +305,7 @@ public static unsafe ushort CompareExchange(ref ushort location1, ushort value, return CompareExchange(ref location1, value, comparand); // Must expand intrinsic #else // this relies on GC keeping 4B alignment for refs and on subtracting to such alignment being in the same object - nuint offset = (nuint)Unsafe.AsPointer(ref location1) % sizeof(uint); + nuint offset = Unsafe.OpportunisticMisalignment(ref location1, sizeof(uint)); ref uint alignedRef = ref Unsafe.As(ref Unsafe.SubtractByteOffset(ref location1, offset)); int bitOffset = (int)((BitConverter.IsLittleEndian ? offset : sizeof(uint) - offset - sizeof(byte)) * 8); // to bit offset @@ -320,7 +320,7 @@ public static unsafe ushort CompareExchange(ref ushort location1, ushort value, do { // make sure the ref is still aligned - Debug.Assert((nuint)Unsafe.AsPointer(ref alignedRef) % sizeof(uint) == 0); + Debug.Assert(Unsafe.IsOpportunisticallyAligned(ref alignedRef, sizeof(uint)); uint otherMemory = originalValue & mask; fullComparand = otherMemory | shiftedComparand; newValue = otherMemory | shiftedValue; From 5d38ca551e16b7f5cc8d29a5d8b20c79011ccc7a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Petryka?= <35800402+MichalPetryka@users.noreply.github.com> Date: Thu, 21 Mar 2024 19:17:25 +0100 Subject: [PATCH 8/8] Fix build errors --- .../src/System/Threading/Interlocked.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/Interlocked.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/Interlocked.cs index 18e9f86cdb7cdc..45ebce76f7b0ee 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Threading/Interlocked.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Threading/Interlocked.cs @@ -98,7 +98,7 @@ public static unsafe byte Exchange(ref byte location1, byte value) do { // make sure the ref is still aligned - Debug.Assert(Unsafe.IsOpportunisticallyAligned(ref alignedRef, sizeof(uint)); + Debug.Assert(Unsafe.IsOpportunisticallyAligned(ref alignedRef, sizeof(uint))); newValue = originalValue & mask | shiftedValue; } while (originalValue != (originalValue = CompareExchange(ref alignedRef, newValue, originalValue))); @@ -137,7 +137,7 @@ public static unsafe ushort Exchange(ref ushort location1, ushort value) do { // make sure the ref is still aligned - Debug.Assert(Unsafe.IsOpportunisticallyAligned(ref alignedRef, sizeof(uint)); + Debug.Assert(Unsafe.IsOpportunisticallyAligned(ref alignedRef, sizeof(uint))); newValue = originalValue & mask | shiftedValue; } while (originalValue != (originalValue = CompareExchange(ref alignedRef, newValue, originalValue))); @@ -277,7 +277,7 @@ public static unsafe byte CompareExchange(ref byte location1, byte value, byte c do { // make sure the ref is still aligned - Debug.Assert(Unsafe.IsOpportunisticallyAligned(ref alignedRef, sizeof(uint)); + Debug.Assert(Unsafe.IsOpportunisticallyAligned(ref alignedRef, sizeof(uint))); uint otherMemory = originalValue & mask; fullComparand = otherMemory | shiftedComparand; newValue = otherMemory | shiftedValue; @@ -320,7 +320,7 @@ public static unsafe ushort CompareExchange(ref ushort location1, ushort value, do { // make sure the ref is still aligned - Debug.Assert(Unsafe.IsOpportunisticallyAligned(ref alignedRef, sizeof(uint)); + Debug.Assert(Unsafe.IsOpportunisticallyAligned(ref alignedRef, sizeof(uint))); uint otherMemory = originalValue & mask; fullComparand = otherMemory | shiftedComparand; newValue = otherMemory | shiftedValue;