Skip to content

Commit

Permalink
Remove uses of FCThrow in interlocked helpers (#96916)
Browse files Browse the repository at this point in the history 
FCThrow is implemented usign HELPER_METHOD_FRAME. All uses of FCThrow need to be removed to fix #95695.
  • Loading branch information
@jkotas
jkotas authored Jan 13, 2024
1 parent eb5f4e6 commit 3e2bc17
Show file tree
Hide file tree
Showing 4 changed files with 115 additions and 57 deletions.
112 changes: 101 additions & 11 deletions src/coreclr/System.Private.CoreLib/src/System/Threading/Interlocked.CoreCLR.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,27 +48,60 @@ public static long Decrement(ref long location) =>
/// <returns>The original value of <paramref name="location1"/>.</returns>
/// <exception cref="NullReferenceException">The address of location1 is a null pointer.</exception>
[Intrinsic]
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static int Exchange(ref int location1, int value)
{
#if TARGET_X86 || TARGET_AMD64 || TARGET_ARM64 || TARGET_RISCV64
return Exchange(ref location1, value); // Must expand intrinsic
#else
if (Unsafe.IsNullRef(ref location1))
ThrowHelper.ThrowNullReferenceException();
return Exchange32(ref location1, value);
#endif
}

[MethodImpl(MethodImplOptions.InternalCall)]
public static extern int Exchange(ref int location1, int value);
private static extern int Exchange32(ref int location1, int value);

/// <summary>Sets a 64-bit signed integer to a specified value and returns the original value, as an atomic operation.</summary>
/// <param name="location1">The variable to set to the specified value.</param>
/// <param name="value">The value to which the <paramref name="location1"/> parameter is set.</param>
/// <returns>The original value of <paramref name="location1"/>.</returns>
/// <exception cref="NullReferenceException">The address of location1 is a null pointer.</exception>
[Intrinsic]
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static long Exchange(ref long location1, long value)
{
#if TARGET_AMD64 || TARGET_ARM64 || TARGET_RISCV64
return Exchange(ref location1, value); // Must expand intrinsic
#else
if (Unsafe.IsNullRef(ref location1))
ThrowHelper.ThrowNullReferenceException();
return Exchange64(ref location1, value);
#endif
}

[MethodImpl(MethodImplOptions.InternalCall)]
public static extern long Exchange(ref long location1, long value);
private static extern long Exchange64(ref long location1, long value);

/// <summary>Sets an object to the specified value and returns a reference to the original object, as an atomic operation.</summary>
/// <param name="location1">The variable to set to the specified value.</param>
/// <param name="value">The value to which the <paramref name="location1"/> parameter is set.</param>
/// <returns>The original value of <paramref name="location1"/>.</returns>
/// <exception cref="NullReferenceException">The address of location1 is a null pointer.</exception>
[Intrinsic]
[MethodImpl(MethodImplOptions.InternalCall)]
[MethodImpl(MethodImplOptions.AggressiveInlining)]
[return: NotNullIfNotNull(nameof(location1))]
public static extern object? Exchange([NotNullIfNotNull(nameof(value))] ref object? location1, object? value);
public static object? Exchange([NotNullIfNotNull(nameof(value))] ref object? location1, object? value)
{
if (Unsafe.IsNullRef(ref location1))
ThrowHelper.ThrowNullReferenceException();
return ExchangeObject(ref location1, value);
}

[return: NotNullIfNotNull(nameof(location1))]
[MethodImpl(MethodImplOptions.InternalCall)]
private static extern object? ExchangeObject([NotNullIfNotNull(nameof(value))] ref object? location1, object? value);

// The below whole method reduces to a single call to Exchange(ref object, object) but
// the JIT thinks that it will generate more native code than it actually does.
Expand All @@ -84,7 +117,7 @@ public static long Decrement(ref long location) =>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static T Exchange<T>([NotNullIfNotNull(nameof(value))] ref T location1, T value) where T : class? =>
Unsafe.As<T>(Exchange(ref Unsafe.As<T, object?>(ref location1), value));
#endregion
#endregion

#region CompareExchange
/// <summary>Compares two 32-bit signed integers for equality and, if they are equal, replaces the first value.</summary>
Expand All @@ -94,8 +127,20 @@ public static T Exchange<T>([NotNullIfNotNull(nameof(value))] ref T location1, T
/// <returns>The original value in <paramref name="location1"/>.</returns>
/// <exception cref="NullReferenceException">The address of <paramref name="location1"/> is a null pointer.</exception>
[Intrinsic]
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static int CompareExchange(ref int location1, int value, int comparand)
{
#if TARGET_X86 || TARGET_AMD64 || TARGET_ARM64 || TARGET_RISCV64
return CompareExchange(ref location1, value, comparand); // Must expand intrinsic
#else
if (Unsafe.IsNullRef(ref location1))
ThrowHelper.ThrowNullReferenceException();
return CompareExchange32(ref location1, value, comparand);
#endif
}

[MethodImpl(MethodImplOptions.InternalCall)]
public static extern int CompareExchange(ref int location1, int value, int comparand);
private static extern int CompareExchange32(ref int location1, int value, int comparand);

/// <summary>Compares two 64-bit signed integers for equality and, if they are equal, replaces the first value.</summary>
/// <param name="location1">The destination, whose value is compared with <paramref name="comparand"/> and possibly replaced.</param>
Expand All @@ -104,8 +149,20 @@ public static T Exchange<T>([NotNullIfNotNull(nameof(value))] ref T location1, T
/// <returns>The original value in <paramref name="location1"/>.</returns>
/// <exception cref="NullReferenceException">The address of <paramref name="location1"/> is a null pointer.</exception>
[Intrinsic]
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static long CompareExchange(ref long location1, long value, long comparand)
{
#if TARGET_AMD64 || TARGET_ARM64 || TARGET_RISCV64
return CompareExchange(ref location1, value, comparand); // Must expand intrinsic
#else
if (Unsafe.IsNullRef(ref location1))
ThrowHelper.ThrowNullReferenceException();
return CompareExchange64(ref location1, value, comparand);
#endif
}

[MethodImpl(MethodImplOptions.InternalCall)]
public static extern long CompareExchange(ref long location1, long value, long comparand);
private static extern long CompareExchange64(ref long location1, long value, long comparand);

/// <summary>Compares two objects for reference equality and, if they are equal, replaces the first object.</summary>
/// <param name="location1">The destination object that is compared by reference with <paramref name="comparand"/> and possibly replaced.</param>
Expand All @@ -114,9 +171,18 @@ public static T Exchange<T>([NotNullIfNotNull(nameof(value))] ref T location1, T
/// <returns>The original value in <paramref name="location1"/>.</returns>
/// <exception cref="NullReferenceException">The address of <paramref name="location1"/> is a null pointer.</exception>
[Intrinsic]
[MethodImpl(MethodImplOptions.AggressiveInlining)]
[return: NotNullIfNotNull(nameof(location1))]
public static object? CompareExchange(ref object? location1, object? value, object? comparand)
{
if (Unsafe.IsNullRef(ref location1))
ThrowHelper.ThrowNullReferenceException();
return CompareExchangeObject(ref location1, value, comparand);
}

[MethodImpl(MethodImplOptions.InternalCall)]
[return: NotNullIfNotNull(nameof(location1))]
public static extern object? CompareExchange(ref object? location1, object? value, object? comparand);
private static extern object? CompareExchangeObject(ref object? location1, object? value, object? comparand);

// Note that getILIntrinsicImplementationForInterlocked() in vm\jitinterface.cpp replaces
// the body of the following method with the following IL:
Expand All @@ -136,8 +202,8 @@ public static T Exchange<T>([NotNullIfNotNull(nameof(value))] ref T location1, T
/// <exception cref="NullReferenceException">The address of <paramref name="location1"/> is a null pointer.</exception>
/// <typeparam name="T">The type to be used for <paramref name="location1"/>, <paramref name="value"/>, and <paramref name="comparand"/>. This type must be a reference type.</typeparam>
[Intrinsic]
[return: NotNullIfNotNull(nameof(location1))]
[MethodImpl(MethodImplOptions.AggressiveInlining)]
[return: NotNullIfNotNull(nameof(location1))]
public static T CompareExchange<T>(ref T location1, T value, T comparand) where T : class? =>
Unsafe.As<T>(CompareExchange(ref Unsafe.As<T, object?>(ref location1), value, comparand));
#endregion
Expand All @@ -160,12 +226,36 @@ public static long Add(ref long location1, long value) =>
ExchangeAdd(ref location1, value) + value;

[Intrinsic]
[MethodImpl(MethodImplOptions.AggressiveInlining)]
private static int ExchangeAdd(ref int location1, int value)
{
#if TARGET_X86 || TARGET_AMD64 || TARGET_ARM64 || TARGET_RISCV64
return ExchangeAdd(ref location1, value); // Must expand intrinsic
#else
if (Unsafe.IsNullRef(ref location1))
ThrowHelper.ThrowNullReferenceException();
return ExchangeAdd32(ref location1, value);
#endif
}

[MethodImpl(MethodImplOptions.InternalCall)]
private static extern int ExchangeAdd(ref int location1, int value);
private static extern int ExchangeAdd32(ref int location1, int value);

[Intrinsic]
[MethodImpl(MethodImplOptions.AggressiveInlining)]
private static long ExchangeAdd(ref long location1, long value)
{
#if TARGET_AMD64 || TARGET_ARM64 || TARGET_RISCV64
return ExchangeAdd(ref location1, value); // Must expand intrinsic
#else
if (Unsafe.IsNullRef(ref location1))
ThrowHelper.ThrowNullReferenceException();
return ExchangeAdd64(ref location1, value);
#endif
}

[MethodImpl(MethodImplOptions.InternalCall)]
private static extern long ExchangeAdd(ref long location1, long value);
private static extern long ExchangeAdd64(ref long location1, long value);
#endregion

#region Read
Expand Down
36 changes: 2 additions & 34 deletions src/coreclr/vm/comutilnative.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1490,14 +1490,10 @@ NOINLINE void GCInterface::GarbageCollectModeAny(int generation)

#include <optsmallperfcritical.h>

FCIMPL2(INT32,COMInterlocked::Exchange, INT32 *location, INT32 value)
FCIMPL2(INT32,COMInterlocked::Exchange32, INT32 *location, INT32 value)
{
FCALL_CONTRACT;

if( NULL == location) {
FCThrow(kNullReferenceException);
}

return InterlockedExchange((LONG *) location, value);
}
FCIMPLEND
Expand All @@ -1506,22 +1502,14 @@ FCIMPL2_IV(INT64,COMInterlocked::Exchange64, INT64 *location, INT64 value)
{
FCALL_CONTRACT;

if( NULL == location) {
FCThrow(kNullReferenceException);
}

return InterlockedExchange64((INT64 *) location, value);
}
FCIMPLEND

FCIMPL3(INT32, COMInterlocked::CompareExchange, INT32* location, INT32 value, INT32 comparand)
FCIMPL3(INT32, COMInterlocked::CompareExchange32, INT32* location, INT32 value, INT32 comparand)
{
FCALL_CONTRACT;

if( NULL == location) {
FCThrow(kNullReferenceException);
}

return InterlockedCompareExchange((LONG*)location, value, comparand);
}
FCIMPLEND
Expand All @@ -1530,10 +1518,6 @@ FCIMPL3_IVV(INT64, COMInterlocked::CompareExchange64, INT64* location, INT64 val
{
FCALL_CONTRACT;

if( NULL == location) {
FCThrow(kNullReferenceException);
}

return InterlockedCompareExchange64((INT64*)location, value, comparand);
}
FCIMPLEND
Expand All @@ -1542,10 +1526,6 @@ FCIMPL2(LPVOID,COMInterlocked::ExchangeObject, LPVOID*location, LPVOID value)
{
FCALL_CONTRACT;

if( NULL == location) {
FCThrow(kNullReferenceException);
}

LPVOID ret = InterlockedExchangeT(location, value);
#ifdef _DEBUG
Thread::ObjectRefAssign((OBJECTREF *)location);
Expand All @@ -1559,10 +1539,6 @@ FCIMPL3(LPVOID,COMInterlocked::CompareExchangeObject, LPVOID *location, LPVOID v
{
FCALL_CONTRACT;

if( NULL == location) {
FCThrow(kNullReferenceException);
}

// <TODO>@todo: only set ref if is updated</TODO>
LPVOID ret = InterlockedCompareExchangeT(location, value, comparand);
if (ret == comparand) {
Expand All @@ -1579,10 +1555,6 @@ FCIMPL2(INT32,COMInterlocked::ExchangeAdd32, INT32 *location, INT32 value)
{
FCALL_CONTRACT;

if( NULL == location) {
FCThrow(kNullReferenceException);
}

return InterlockedExchangeAdd((LONG *) location, value);
}
FCIMPLEND
Expand All @@ -1591,10 +1563,6 @@ FCIMPL2_IV(INT64,COMInterlocked::ExchangeAdd64, INT64 *location, INT64 value)
{
FCALL_CONTRACT;

if( NULL == location) {
FCThrow(kNullReferenceException);
}

return InterlockedExchangeAdd64((INT64 *) location, value);
}
FCIMPLEND
Expand Down
8 changes: 4 additions & 4 deletions src/coreclr/vm/comutilnative.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -230,10 +230,10 @@ extern "C" uint64_t QCALLTYPE GCInterface_GetGenerationBudget(int generation);
class COMInterlocked
{
public:
static FCDECL2(INT32, Exchange, INT32 *location, INT32 value);
static FCDECL2_IV(INT64, Exchange64, INT64 *location, INT64 value);
static FCDECL3(INT32, CompareExchange, INT32* location, INT32 value, INT32 comparand);
static FCDECL3_IVV(INT64, CompareExchange64, INT64* location, INT64 value, INT64 comparand);
static FCDECL2(INT32, Exchange32, INT32 *location, INT32 value);
static FCDECL2_IV(INT64, Exchange64, INT64 *location, INT64 value);
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);
static FCDECL3(LPVOID, CompareExchangeObject, LPVOID* location, LPVOID value, LPVOID comparand);
static FCDECL2(INT32, ExchangeAdd32, INT32 *location, INT32 value);
Expand Down
16 changes: 8 additions & 8 deletions src/coreclr/vm/ecalllist.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -457,14 +457,14 @@ FCFuncStart(gInteropMarshalFuncs)
FCFuncEnd()

FCFuncStart(gInterlockedFuncs)
FCFuncElementSig("Exchange", &gsig_SM_RefInt_Int_RetInt, COMInterlocked::Exchange)
FCFuncElementSig("Exchange", &gsig_SM_RefLong_Long_RetLong, COMInterlocked::Exchange64)
FCFuncElementSig("Exchange", &gsig_SM_RefObj_Obj_RetObj, COMInterlocked::ExchangeObject)
FCFuncElementSig("CompareExchange", &gsig_SM_RefInt_Int_Int_RetInt, COMInterlocked::CompareExchange)
FCFuncElementSig("CompareExchange", &gsig_SM_RefLong_Long_Long_RetLong, COMInterlocked::CompareExchange64)
FCFuncElementSig("CompareExchange", &gsig_SM_RefObj_Obj_Obj_RetObj, COMInterlocked::CompareExchangeObject)
FCFuncElementSig("ExchangeAdd", &gsig_SM_RefInt_Int_RetInt, COMInterlocked::ExchangeAdd32)
FCFuncElementSig("ExchangeAdd", &gsig_SM_RefLong_Long_RetLong, COMInterlocked::ExchangeAdd64)
FCFuncElement("Exchange32", COMInterlocked::Exchange32)
FCFuncElement("Exchange64", COMInterlocked::Exchange64)
FCFuncElement("ExchangeObject", COMInterlocked::ExchangeObject)
FCFuncElement("CompareExchange32", COMInterlocked::CompareExchange32)
FCFuncElement("CompareExchange64", COMInterlocked::CompareExchange64)
FCFuncElement("CompareExchangeObject", COMInterlocked::CompareExchangeObject)
FCFuncElement("ExchangeAdd32", COMInterlocked::ExchangeAdd32)
FCFuncElement("ExchangeAdd64", COMInterlocked::ExchangeAdd64)
FCFuncEnd()

FCFuncStart(gJitInfoFuncs)
Expand Down

0 comments on commit 3e2bc17

Please sign in to comment.