Skip to content

Commit

Permalink
Unify Default Comparer logic (#88006)
Browse files Browse the repository at this point in the history 
Unifies (Equality)Comparer.Default logic so that it matches between CoreCLR, NativeAOT, Mono in runtime and intrinsic implementations.

Fixes devirt behaviour around Nullable types.

Also enables devirt for non final types in CoreCLR and NativeAOT.

Required for #87579.
Fixes #87391.

Co-authored-by: Jan Kotas <[email protected]>
  • Loading branch information
@MichalPetryka @jkotas
MichalPetryka and jkotas authored Jul 5, 2023
1 parent e4da7e9 commit 18321c6
Show file tree
Hide file tree
Showing 13 changed files with 381 additions and 238 deletions.
77 changes: 4 additions & 73 deletions src/coreclr/System.Private.CoreLib/src/System/Collections/Generic/ComparerHelpers.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,44 +46,12 @@ internal static object CreateDefaultComparer(Type type)
// The comparer for enums is specialized to avoid boxing.
else if (type.IsEnum)
{
result = TryCreateEnumComparer(runtimeType);
result = CreateInstanceForAnotherGenericParameter((RuntimeType)typeof(EnumComparer<>), runtimeType);
}

return result ?? CreateInstanceForAnotherGenericParameter((RuntimeType)typeof(ObjectComparer<object>), runtimeType);
}

/// <summary>
/// Creates the default <see cref="Comparer{T}"/> for an enum type.
/// </summary>
/// <param name="enumType">The enum type to create the default comparer for.</param>
private static object? TryCreateEnumComparer(RuntimeType enumType)
{
Debug.Assert(enumType != null);
Debug.Assert(enumType.IsEnum);

// Explicitly call Enum.GetUnderlyingType here. Although GetTypeCode
// ends up doing this anyway, we end up avoiding an unnecessary P/Invoke
// and virtual method call.
TypeCode underlyingTypeCode = Type.GetTypeCode(Enum.GetUnderlyingType(enumType));

// Depending on the enum type, we need to special case the comparers so that we avoid boxing.
// Specialize differently for signed/unsigned types so we avoid problems with large numbers.
switch (underlyingTypeCode)
{
case TypeCode.SByte:
case TypeCode.Int16:
case TypeCode.Int32:
case TypeCode.Byte:
case TypeCode.UInt16:
case TypeCode.UInt32:
case TypeCode.Int64:
case TypeCode.UInt64:
return CreateInstanceForAnotherGenericParameter((RuntimeType)typeof(EnumComparer<>), enumType);
}

return null;
}

/// <summary>
/// Creates the default <see cref="EqualityComparer{T}"/>.
/// </summary>
Expand All @@ -98,15 +66,9 @@ internal static object CreateDefaultEqualityComparer(Type type)
object? result = null;
var runtimeType = (RuntimeType)type;

if (type == typeof(byte))
{
// Specialize for byte so Array.IndexOf is faster.
result = new ByteEqualityComparer();
}
else if (type == typeof(string))
if (type == typeof(string))
{
// Specialize for string, as EqualityComparer<string>.Default is on the startup path
result = new GenericEqualityComparer<string>();
return new GenericEqualityComparer<string>();
}
else if (type.IsAssignableTo(typeof(IEquatable<>).MakeGenericType(type)))
{
Expand All @@ -122,41 +84,10 @@ internal static object CreateDefaultEqualityComparer(Type type)
else if (type.IsEnum)
{
// The equality comparer for enums is specialized to avoid boxing.
result = TryCreateEnumEqualityComparer(runtimeType);
result = CreateInstanceForAnotherGenericParameter((RuntimeType)typeof(EnumEqualityComparer<>), runtimeType);
}

return result ?? CreateInstanceForAnotherGenericParameter((RuntimeType)typeof(ObjectEqualityComparer<object>), runtimeType);
}

/// <summary>
/// Creates the default <see cref="EqualityComparer{T}"/> for an enum type.
/// </summary>
/// <param name="enumType">The enum type to create the default equality comparer for.</param>
private static object? TryCreateEnumEqualityComparer(RuntimeType enumType)
{
Debug.Assert(enumType != null);
Debug.Assert(enumType.IsEnum);

// See the METHOD__JIT_HELPERS__UNSAFE_ENUM_CAST and METHOD__JIT_HELPERS__UNSAFE_ENUM_CAST_LONG cases in getILIntrinsicImplementation
// for how we cast the enum types to integral values in the comparer without boxing.

TypeCode underlyingTypeCode = Type.GetTypeCode(Enum.GetUnderlyingType(enumType));

// Depending on the enum type, we need to special case the comparers so that we avoid boxing.
switch (underlyingTypeCode)
{
case TypeCode.Int32:
case TypeCode.UInt32:
case TypeCode.SByte:
case TypeCode.Byte:
case TypeCode.Int16:
case TypeCode.Int64:
case TypeCode.UInt64:
case TypeCode.UInt16:
return CreateInstanceForAnotherGenericParameter((RuntimeType)typeof(EnumEqualityComparer<>), enumType);
}

return null;
}
}
}
56 changes: 20 additions & 36 deletions src/coreclr/jit/importercalls.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7553,57 +7553,41 @@ CORINFO_CLASS_HANDLE Compiler::impGetSpecialIntrinsicExactReturnType(GenTreeCall
CORINFO_SIG_INFO sig;
info.compCompHnd->getMethodSig(methodHnd, &sig);
assert(sig.sigInst.classInstCount == 1);

CORINFO_CLASS_HANDLE typeHnd = sig.sigInst.classInst[0];
assert(typeHnd != nullptr);

// Lookup can incorrect when we have __Canon as it won't appear
// to implement any interface types.
//
// And if we do not have a final type, devirt & inlining is
// unlikely to result in much simplification.
//
// We can use CORINFO_FLG_FINAL to screen out both of these cases.
const DWORD typeAttribs = info.compCompHnd->getClassAttribs(typeHnd);
bool isFinalType = ((typeAttribs & CORINFO_FLG_FINAL) != 0);

if (!isFinalType)
CallArg* instParam = call->gtArgs.FindWellKnownArg(WellKnownArg::InstParam);
if (instParam != nullptr)
{
CallArg* instParam = call->gtArgs.FindWellKnownArg(WellKnownArg::InstParam);
if (instParam != nullptr)
assert(instParam->GetNext() == nullptr);
CORINFO_CLASS_HANDLE hClass = gtGetHelperArgClassHandle(instParam->GetNode());
if (hClass != NO_CLASS_HANDLE)
{
assert(instParam->GetNext() == nullptr);
CORINFO_CLASS_HANDLE hClass = gtGetHelperArgClassHandle(instParam->GetNode());
if (hClass != NO_CLASS_HANDLE)
{
hClass = getTypeInstantiationArgument(hClass, 0);
if ((info.compCompHnd->getClassAttribs(hClass) & CORINFO_FLG_FINAL) != 0)
{
typeHnd = hClass;
isFinalType = true;
}
}
typeHnd = getTypeInstantiationArgument(hClass, 0);
}
}

if (isFinalType)
if (ni == NI_System_Collections_Generic_EqualityComparer_get_Default)
{
result = info.compCompHnd->getDefaultEqualityComparerClass(typeHnd);
}
else
{
assert(ni == NI_System_Collections_Generic_Comparer_get_Default);
result = info.compCompHnd->getDefaultComparerClass(typeHnd);
}

if (result != NO_CLASS_HANDLE)
{
if (ni == NI_System_Collections_Generic_EqualityComparer_get_Default)
{
result = info.compCompHnd->getDefaultEqualityComparerClass(typeHnd);
}
else
{
assert(ni == NI_System_Collections_Generic_Comparer_get_Default);
result = info.compCompHnd->getDefaultComparerClass(typeHnd);
}
JITDUMP("Special intrinsic for type %s: return type is %s\n", eeGetClassName(typeHnd),
result != nullptr ? eeGetClassName(result) : "unknown");
}
else
{
JITDUMP("Special intrinsic for type %s: type not final, so deferring opt\n", eeGetClassName(typeHnd));
JITDUMP("Special intrinsic for type %s: type undetermined, so deferring opt\n",
eeGetClassName(typeHnd));
}

break;
}

Expand Down
1 change: 0 additions & 1 deletion src/coreclr/vm/corelib.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1155,7 +1155,6 @@ DEFINE_METHOD(UTF8BUFFERMARSHALER, CONVERT_TO_MANAGED, ConvertToManaged, NoSig)

// Classes referenced in EqualityComparer<T>.Default optimization

DEFINE_CLASS(BYTE_EQUALITYCOMPARER, CollectionsGeneric, ByteEqualityComparer)
DEFINE_CLASS(ENUM_EQUALITYCOMPARER, CollectionsGeneric, EnumEqualityComparer`1)
DEFINE_CLASS(NULLABLE_EQUALITYCOMPARER, CollectionsGeneric, NullableEqualityComparer`1)
DEFINE_CLASS(GENERIC_EQUALITYCOMPARER, CollectionsGeneric, GenericEqualityComparer`1)
Expand Down
102 changes: 26 additions & 76 deletions src/coreclr/vm/jitinterface.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8824,55 +8824,31 @@ CORINFO_CLASS_HANDLE CEEInfo::getDefaultComparerClassHelper(CORINFO_CLASS_HANDLE
// We need to find the appropriate instantiation
Instantiation inst(&elemTypeHnd, 1);

// If T implements IComparable<T>
if (elemTypeHnd.CanCastTo(TypeHandle(CoreLibBinder::GetClass(CLASS__ICOMPARABLEGENERIC)).Instantiate(inst)))
{
TypeHandle resultTh = ((TypeHandle)CoreLibBinder::GetClass(CLASS__GENERIC_COMPARER)).Instantiate(inst);
return CORINFO_CLASS_HANDLE(resultTh.GetMethodTable());
}

// Nullable<T>
if (Nullable::IsNullableType(elemTypeHnd))
{
Instantiation nullableInst = elemTypeHnd.AsMethodTable()->GetInstantiation();
TypeHandle iequatable = TypeHandle(CoreLibBinder::GetClass(CLASS__IEQUATABLEGENERIC)).Instantiate(nullableInst);
if (nullableInst[0].CanCastTo(iequatable))
{
TypeHandle resultTh = ((TypeHandle)CoreLibBinder::GetClass(CLASS__NULLABLE_COMPARER)).Instantiate(nullableInst);
return CORINFO_CLASS_HANDLE(resultTh.GetMethodTable());
}
TypeHandle resultTh = ((TypeHandle)CoreLibBinder::GetClass(CLASS__NULLABLE_COMPARER)).Instantiate(nullableInst);
return CORINFO_CLASS_HANDLE(resultTh.GetMethodTable());
}

// We need to special case the Enum comparers based on their underlying type to avoid boxing
if (elemTypeHnd.IsEnum())
{
MethodTable* targetClass = NULL;
CorElementType normType = elemTypeHnd.GetVerifierCorElementType();

switch(normType)
{
case ELEMENT_TYPE_I1:
case ELEMENT_TYPE_I2:
case ELEMENT_TYPE_U1:
case ELEMENT_TYPE_U2:
case ELEMENT_TYPE_I4:
case ELEMENT_TYPE_U4:
case ELEMENT_TYPE_I8:
case ELEMENT_TYPE_U8:
{
targetClass = CoreLibBinder::GetClass(CLASS__ENUM_COMPARER);
break;
}
TypeHandle resultTh = ((TypeHandle)CoreLibBinder::GetClass(CLASS__ENUM_COMPARER)).Instantiate(inst);
return CORINFO_CLASS_HANDLE(resultTh.GetMethodTable());
}

default:
break;
}
if (elemTypeHnd.IsCanonicalSubtype())
{
return NULL;
}

if (targetClass != NULL)
{
TypeHandle resultTh = ((TypeHandle)targetClass->GetCanonicalMethodTable()).Instantiate(inst);
return CORINFO_CLASS_HANDLE(resultTh.GetMethodTable());
}
// If T implements IComparable<T>
if (elemTypeHnd.CanCastTo(TypeHandle(CoreLibBinder::GetClass(CLASS__ICOMPARABLEGENERIC)).Instantiate(inst)))
{
TypeHandle resultTh = ((TypeHandle)CoreLibBinder::GetClass(CLASS__GENERIC_COMPARER)).Instantiate(inst);
return CORINFO_CLASS_HANDLE(resultTh.GetMethodTable());
}

// Default case
Expand Down Expand Up @@ -8913,25 +8889,12 @@ CORINFO_CLASS_HANDLE CEEInfo::getDefaultEqualityComparerClassHelper(CORINFO_CLAS
// And in compile.cpp's SpecializeEqualityComparer
TypeHandle elemTypeHnd(elemType);

// Special case for byte
if (elemTypeHnd.AsMethodTable()->HasSameTypeDefAs(CoreLibBinder::GetClass(CLASS__ELEMENT_TYPE_U1)))
{
return CORINFO_CLASS_HANDLE(CoreLibBinder::GetClass(CLASS__BYTE_EQUALITYCOMPARER));
}

// Mirrors the logic in BCL's CompareHelpers.CreateDefaultComparer
// And in compile.cpp's SpecializeComparer
//
// We need to find the appropriate instantiation
Instantiation inst(&elemTypeHnd, 1);

// If T implements IEquatable<T>
if (elemTypeHnd.CanCastTo(TypeHandle(CoreLibBinder::GetClass(CLASS__IEQUATABLEGENERIC)).Instantiate(inst)))
{
TypeHandle resultTh = ((TypeHandle)CoreLibBinder::GetClass(CLASS__GENERIC_EQUALITYCOMPARER)).Instantiate(inst);
return CORINFO_CLASS_HANDLE(resultTh.GetMethodTable());
}

// Nullable<T>
if (Nullable::IsNullableType(elemTypeHnd))
{
Expand All @@ -8946,33 +8909,20 @@ CORINFO_CLASS_HANDLE CEEInfo::getDefaultEqualityComparerClassHelper(CORINFO_CLAS
// to avoid boxing and call the correct versions of GetHashCode.
if (elemTypeHnd.IsEnum())
{
MethodTable* targetClass = NULL;
CorElementType normType = elemTypeHnd.GetVerifierCorElementType();

switch(normType)
{
case ELEMENT_TYPE_I1:
case ELEMENT_TYPE_I2:
case ELEMENT_TYPE_U1:
case ELEMENT_TYPE_U2:
case ELEMENT_TYPE_I4:
case ELEMENT_TYPE_U4:
case ELEMENT_TYPE_I8:
case ELEMENT_TYPE_U8:
{
targetClass = CoreLibBinder::GetClass(CLASS__ENUM_EQUALITYCOMPARER);
break;
}
TypeHandle resultTh = ((TypeHandle)CoreLibBinder::GetClass(CLASS__ENUM_EQUALITYCOMPARER)).Instantiate(inst);
return CORINFO_CLASS_HANDLE(resultTh.GetMethodTable());
}

default:
break;
}
if (elemTypeHnd.IsCanonicalSubtype())
{
return NULL;
}

if (targetClass != NULL)
{
TypeHandle resultTh = ((TypeHandle)targetClass->GetCanonicalMethodTable()).Instantiate(inst);
return CORINFO_CLASS_HANDLE(resultTh.GetMethodTable());
}
// If T implements IEquatable<T>
if (elemTypeHnd.CanCastTo(TypeHandle(CoreLibBinder::GetClass(CLASS__IEQUATABLEGENERIC)).Instantiate(inst)))
{
TypeHandle resultTh = ((TypeHandle)CoreLibBinder::GetClass(CLASS__GENERIC_EQUALITYCOMPARER)).Instantiate(inst);
return CORINFO_CLASS_HANDLE(resultTh.GetMethodTable());
}

// Default case
Expand Down
6 changes: 1 addition & 5 deletions src/mono/System.Private.CoreLib/src/System/Collections/Generic/EqualityComparer.Mono.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,7 @@ private static EqualityComparer<T> CreateComparer()
// IN mini_handle_call_res_devirt
/////////////////////////////////////////////////

if (t == typeof(byte))
{
return (EqualityComparer<T>)(object)(new ByteEqualityComparer());
}
else if (t == typeof(string))
if (t == typeof(string))
{
// Specialize for string, as EqualityComparer<string>.Default is on the startup path
return (EqualityComparer<T>)(object)(new GenericEqualityComparer<string>());
Expand Down
6 changes: 2 additions & 4 deletions src/mono/System.Private.CoreLib/src/System/Runtime/CompilerServices/JitHelpers.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,11 @@ namespace System.Runtime.CompilerServices
{
internal static class JitHelpers
{
#pragma warning disable IDE0060
[Intrinsic]
public static bool EnumEquals<T>(T x, T y) where T : struct, Enum => throw new NotImplementedException();
public static bool EnumEquals<T>(T x, T y) where T : struct, Enum => x.Equals(y);

[Intrinsic]
public static int EnumCompareTo<T>(T x, T y) where T : struct, Enum => throw new NotImplementedException();
#pragma warning restore IDE0060
public static int EnumCompareTo<T>(T x, T y) where T : struct, Enum => x.CompareTo(y);

[Intrinsic]
internal static void DisableInline () => throw new NotImplementedException();
Expand Down
5 changes: 4 additions & 1 deletion src/mono/mono/mini/interp/transform.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2540,7 +2540,10 @@ interp_handle_intrinsics (TransformData *td, MonoMethod *target_method, MonoClas
MonoType *t = ctx->method_inst->type_argv [0];
t = mini_get_underlying_type (t);

gboolean is_i8 = (t->type == MONO_TYPE_I8 || t->type == MONO_TYPE_U8);
if (t->type == MONO_TYPE_R4 || t->type == MONO_TYPE_R8)
return FALSE;

gboolean is_i8 = (t->type == MONO_TYPE_I8 || t->type == MONO_TYPE_U8 || (TARGET_SIZEOF_VOID_P == 8 && (t->type == MONO_TYPE_I || t->type == MONO_TYPE_U)));
gboolean is_unsigned = (t->type == MONO_TYPE_U1 || t->type == MONO_TYPE_U2 || t->type == MONO_TYPE_U4 || t->type == MONO_TYPE_U8 || t->type == MONO_TYPE_U);

gboolean is_compareto = strcmp (tm, "EnumCompareTo") == 0;
Expand Down
4 changes: 2 additions & 2 deletions src/mono/mono/mini/intrinsics.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -672,10 +672,10 @@ emit_jit_helpers_intrinsics (MonoCompile *cfg, MonoMethod *cmethod, MonoMethodSi

t = ctx->method_inst->type_argv [0];
t = mini_get_underlying_type (t);
if (mini_is_gsharedvt_variable_type (t))
if (mini_is_gsharedvt_variable_type (t) || t->type == MONO_TYPE_R4 || t->type == MONO_TYPE_R8)
return NULL;

gboolean is_i8 = (t->type == MONO_TYPE_I8 || t->type == MONO_TYPE_U8);
gboolean is_i8 = (t->type == MONO_TYPE_I8 || t->type == MONO_TYPE_U8 || (TARGET_SIZEOF_VOID_P == 8 && (t->type == MONO_TYPE_I || t->type == MONO_TYPE_U)));
gboolean is_unsigned = (t->type == MONO_TYPE_U1 || t->type == MONO_TYPE_U2 || t->type == MONO_TYPE_U4 || t->type == MONO_TYPE_U8 || t->type == MONO_TYPE_U);
int cmp_op, ceq_op, cgt_op, clt_op;

Expand Down
2 changes: 1 addition & 1 deletion src/mono/mono/mini/mini.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4301,7 +4301,7 @@ mini_handle_call_res_devirt (MonoMethod *cmethod)

// EqualityComparer<T>.Default returns specific types depending on T
// FIXME: Special case more types: byte, string, nullable, enum ?
if (mono_class_is_assignable_from_internal (inst, mono_class_from_mono_type_internal (param_type)) && param_type->type != MONO_TYPE_U1 && param_type->type != MONO_TYPE_STRING) {
if (mono_class_is_assignable_from_internal (inst, mono_class_from_mono_type_internal (param_type)) && param_type->type != MONO_TYPE_STRING) {
MonoClass *gcomparer_inst;

memset (&ctx, 0, sizeof (ctx));
Expand Down
Loading

0 comments on commit 18321c6

Please sign in to comment.