Skip to content

Commit 18321c6

Browse files
MichalPetrykajkotas
MichalPetryka
and
jkotas
authored
Unify Default Comparer logic (#88006)
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 <jkotas@microsoft.com>
1 parent e4da7e9 commit 18321c6

File tree

13 files changed

+381
-238
lines changed

13 files changed

+381
-238
lines changed

‎src/coreclr/System.Private.CoreLib/src/System/Collections/Generic/ComparerHelpers.cs

Lines changed: 4 additions & 73 deletions
Original file line numberDiff line numberDiff line change
@@ -46,44 +46,12 @@ internal static object CreateDefaultComparer(Type type)
4646
// The comparer for enums is specialized to avoid boxing.
4747
else if (type.IsEnum)
4848
{
49-
result = TryCreateEnumComparer(runtimeType);
49+
result = CreateInstanceForAnotherGenericParameter((RuntimeType)typeof(EnumComparer<>), runtimeType);
5050
}
5151

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

55-
/// <summary>
56-
/// Creates the default <see cref="Comparer{T}"/> for an enum type.
57-
/// </summary>
58-
/// <param name="enumType">The enum type to create the default comparer for.</param>
59-
private static object? TryCreateEnumComparer(RuntimeType enumType)
60-
{
61-
Debug.Assert(enumType != null);
62-
Debug.Assert(enumType.IsEnum);
63-
64-
// Explicitly call Enum.GetUnderlyingType here. Although GetTypeCode
65-
// ends up doing this anyway, we end up avoiding an unnecessary P/Invoke
66-
// and virtual method call.
67-
TypeCode underlyingTypeCode = Type.GetTypeCode(Enum.GetUnderlyingType(enumType));
68-
69-
// Depending on the enum type, we need to special case the comparers so that we avoid boxing.
70-
// Specialize differently for signed/unsigned types so we avoid problems with large numbers.
71-
switch (underlyingTypeCode)
72-
{
73-
case TypeCode.SByte:
74-
case TypeCode.Int16:
75-
case TypeCode.Int32:
76-
case TypeCode.Byte:
77-
case TypeCode.UInt16:
78-
case TypeCode.UInt32:
79-
case TypeCode.Int64:
80-
case TypeCode.UInt64:
81-
return CreateInstanceForAnotherGenericParameter((RuntimeType)typeof(EnumComparer<>), enumType);
82-
}
83-
84-
return null;
85-
}
86-
8755
/// <summary>
8856
/// Creates the default <see cref="EqualityComparer{T}"/>.
8957
/// </summary>
@@ -98,15 +66,9 @@ internal static object CreateDefaultEqualityComparer(Type type)
9866
object? result = null;
9967
var runtimeType = (RuntimeType)type;
10068

101-
if (type == typeof(byte))
102-
{
103-
// Specialize for byte so Array.IndexOf is faster.
104-
result = new ByteEqualityComparer();
105-
}
106-
else if (type == typeof(string))
69+
if (type == typeof(string))
10770
{
108-
// Specialize for string, as EqualityComparer<string>.Default is on the startup path
109-
result = new GenericEqualityComparer<string>();
71+
return new GenericEqualityComparer<string>();
11072
}
11173
else if (type.IsAssignableTo(typeof(IEquatable<>).MakeGenericType(type)))
11274
{
@@ -122,41 +84,10 @@ internal static object CreateDefaultEqualityComparer(Type type)
12284
else if (type.IsEnum)
12385
{
12486
// The equality comparer for enums is specialized to avoid boxing.
125-
result = TryCreateEnumEqualityComparer(runtimeType);
87+
result = CreateInstanceForAnotherGenericParameter((RuntimeType)typeof(EnumEqualityComparer<>), runtimeType);
12688
}
12789

12890
return result ?? CreateInstanceForAnotherGenericParameter((RuntimeType)typeof(ObjectEqualityComparer<object>), runtimeType);
12991
}
130-
131-
/// <summary>
132-
/// Creates the default <see cref="EqualityComparer{T}"/> for an enum type.
133-
/// </summary>
134-
/// <param name="enumType">The enum type to create the default equality comparer for.</param>
135-
private static object? TryCreateEnumEqualityComparer(RuntimeType enumType)
136-
{
137-
Debug.Assert(enumType != null);
138-
Debug.Assert(enumType.IsEnum);
139-
140-
// See the METHOD__JIT_HELPERS__UNSAFE_ENUM_CAST and METHOD__JIT_HELPERS__UNSAFE_ENUM_CAST_LONG cases in getILIntrinsicImplementation
141-
// for how we cast the enum types to integral values in the comparer without boxing.
142-
143-
TypeCode underlyingTypeCode = Type.GetTypeCode(Enum.GetUnderlyingType(enumType));
144-
145-
// Depending on the enum type, we need to special case the comparers so that we avoid boxing.
146-
switch (underlyingTypeCode)
147-
{
148-
case TypeCode.Int32:
149-
case TypeCode.UInt32:
150-
case TypeCode.SByte:
151-
case TypeCode.Byte:
152-
case TypeCode.Int16:
153-
case TypeCode.Int64:
154-
case TypeCode.UInt64:
155-
case TypeCode.UInt16:
156-
return CreateInstanceForAnotherGenericParameter((RuntimeType)typeof(EnumEqualityComparer<>), enumType);
157-
}
158-
159-
return null;
160-
}
16192
}
16293
}

‎src/coreclr/jit/importercalls.cpp

Lines changed: 20 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -7553,57 +7553,41 @@ CORINFO_CLASS_HANDLE Compiler::impGetSpecialIntrinsicExactReturnType(GenTreeCall
75537553
CORINFO_SIG_INFO sig;
75547554
info.compCompHnd->getMethodSig(methodHnd, &sig);
75557555
assert(sig.sigInst.classInstCount == 1);
7556+
75567557
CORINFO_CLASS_HANDLE typeHnd = sig.sigInst.classInst[0];
75577558
assert(typeHnd != nullptr);
75587559

7559-
// Lookup can incorrect when we have __Canon as it won't appear
7560-
// to implement any interface types.
7561-
//
7562-
// And if we do not have a final type, devirt & inlining is
7563-
// unlikely to result in much simplification.
7564-
//
7565-
// We can use CORINFO_FLG_FINAL to screen out both of these cases.
7566-
const DWORD typeAttribs = info.compCompHnd->getClassAttribs(typeHnd);
7567-
bool isFinalType = ((typeAttribs & CORINFO_FLG_FINAL) != 0);
7568-
7569-
if (!isFinalType)
7560+
CallArg* instParam = call->gtArgs.FindWellKnownArg(WellKnownArg::InstParam);
7561+
if (instParam != nullptr)
75707562
{
7571-
CallArg* instParam = call->gtArgs.FindWellKnownArg(WellKnownArg::InstParam);
7572-
if (instParam != nullptr)
7563+
assert(instParam->GetNext() == nullptr);
7564+
CORINFO_CLASS_HANDLE hClass = gtGetHelperArgClassHandle(instParam->GetNode());
7565+
if (hClass != NO_CLASS_HANDLE)
75737566
{
7574-
assert(instParam->GetNext() == nullptr);
7575-
CORINFO_CLASS_HANDLE hClass = gtGetHelperArgClassHandle(instParam->GetNode());
7576-
if (hClass != NO_CLASS_HANDLE)
7577-
{
7578-
hClass = getTypeInstantiationArgument(hClass, 0);
7579-
if ((info.compCompHnd->getClassAttribs(hClass) & CORINFO_FLG_FINAL) != 0)
7580-
{
7581-
typeHnd = hClass;
7582-
isFinalType = true;
7583-
}
7584-
}
7567+
typeHnd = getTypeInstantiationArgument(hClass, 0);
75857568
}
75867569
}
75877570

7588-
if (isFinalType)
7571+
if (ni == NI_System_Collections_Generic_EqualityComparer_get_Default)
7572+
{
7573+
result = info.compCompHnd->getDefaultEqualityComparerClass(typeHnd);
7574+
}
7575+
else
7576+
{
7577+
assert(ni == NI_System_Collections_Generic_Comparer_get_Default);
7578+
result = info.compCompHnd->getDefaultComparerClass(typeHnd);
7579+
}
7580+
7581+
if (result != NO_CLASS_HANDLE)
75897582
{
7590-
if (ni == NI_System_Collections_Generic_EqualityComparer_get_Default)
7591-
{
7592-
result = info.compCompHnd->getDefaultEqualityComparerClass(typeHnd);
7593-
}
7594-
else
7595-
{
7596-
assert(ni == NI_System_Collections_Generic_Comparer_get_Default);
7597-
result = info.compCompHnd->getDefaultComparerClass(typeHnd);
7598-
}
75997583
JITDUMP("Special intrinsic for type %s: return type is %s\n", eeGetClassName(typeHnd),
76007584
result != nullptr ? eeGetClassName(result) : "unknown");
76017585
}
76027586
else
76037587
{
7604-
JITDUMP("Special intrinsic for type %s: type not final, so deferring opt\n", eeGetClassName(typeHnd));
7588+
JITDUMP("Special intrinsic for type %s: type undetermined, so deferring opt\n",
7589+
eeGetClassName(typeHnd));
76057590
}
7606-
76077591
break;
76087592
}
76097593

‎src/coreclr/vm/corelib.h

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1155,7 +1155,6 @@ DEFINE_METHOD(UTF8BUFFERMARSHALER, CONVERT_TO_MANAGED, ConvertToManaged, NoSig)
11551155

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

1158-
DEFINE_CLASS(BYTE_EQUALITYCOMPARER, CollectionsGeneric, ByteEqualityComparer)
11591158
DEFINE_CLASS(ENUM_EQUALITYCOMPARER, CollectionsGeneric, EnumEqualityComparer`1)
11601159
DEFINE_CLASS(NULLABLE_EQUALITYCOMPARER, CollectionsGeneric, NullableEqualityComparer`1)
11611160
DEFINE_CLASS(GENERIC_EQUALITYCOMPARER, CollectionsGeneric, GenericEqualityComparer`1)

‎src/coreclr/vm/jitinterface.cpp

Lines changed: 26 additions & 76 deletions
Original file line numberDiff line numberDiff line change
@@ -8824,55 +8824,31 @@ CORINFO_CLASS_HANDLE CEEInfo::getDefaultComparerClassHelper(CORINFO_CLASS_HANDLE
88248824
// We need to find the appropriate instantiation
88258825
Instantiation inst(&elemTypeHnd, 1);
88268826

8827-
// If T implements IComparable<T>
8828-
if (elemTypeHnd.CanCastTo(TypeHandle(CoreLibBinder::GetClass(CLASS__ICOMPARABLEGENERIC)).Instantiate(inst)))
8829-
{
8830-
TypeHandle resultTh = ((TypeHandle)CoreLibBinder::GetClass(CLASS__GENERIC_COMPARER)).Instantiate(inst);
8831-
return CORINFO_CLASS_HANDLE(resultTh.GetMethodTable());
8832-
}
8833-
88348827
// Nullable<T>
88358828
if (Nullable::IsNullableType(elemTypeHnd))
88368829
{
88378830
Instantiation nullableInst = elemTypeHnd.AsMethodTable()->GetInstantiation();
8838-
TypeHandle iequatable = TypeHandle(CoreLibBinder::GetClass(CLASS__IEQUATABLEGENERIC)).Instantiate(nullableInst);
8839-
if (nullableInst[0].CanCastTo(iequatable))
8840-
{
8841-
TypeHandle resultTh = ((TypeHandle)CoreLibBinder::GetClass(CLASS__NULLABLE_COMPARER)).Instantiate(nullableInst);
8842-
return CORINFO_CLASS_HANDLE(resultTh.GetMethodTable());
8843-
}
8831+
TypeHandle resultTh = ((TypeHandle)CoreLibBinder::GetClass(CLASS__NULLABLE_COMPARER)).Instantiate(nullableInst);
8832+
return CORINFO_CLASS_HANDLE(resultTh.GetMethodTable());
88448833
}
88458834

88468835
// We need to special case the Enum comparers based on their underlying type to avoid boxing
88478836
if (elemTypeHnd.IsEnum())
88488837
{
8849-
MethodTable* targetClass = NULL;
8850-
CorElementType normType = elemTypeHnd.GetVerifierCorElementType();
8851-
8852-
switch(normType)
8853-
{
8854-
case ELEMENT_TYPE_I1:
8855-
case ELEMENT_TYPE_I2:
8856-
case ELEMENT_TYPE_U1:
8857-
case ELEMENT_TYPE_U2:
8858-
case ELEMENT_TYPE_I4:
8859-
case ELEMENT_TYPE_U4:
8860-
case ELEMENT_TYPE_I8:
8861-
case ELEMENT_TYPE_U8:
8862-
{
8863-
targetClass = CoreLibBinder::GetClass(CLASS__ENUM_COMPARER);
8864-
break;
8865-
}
8838+
TypeHandle resultTh = ((TypeHandle)CoreLibBinder::GetClass(CLASS__ENUM_COMPARER)).Instantiate(inst);
8839+
return CORINFO_CLASS_HANDLE(resultTh.GetMethodTable());
8840+
}
88668841

8867-
default:
8868-
break;
8869-
}
8842+
if (elemTypeHnd.IsCanonicalSubtype())
8843+
{
8844+
return NULL;
8845+
}
88708846

8871-
if (targetClass != NULL)
8872-
{
8873-
TypeHandle resultTh = ((TypeHandle)targetClass->GetCanonicalMethodTable()).Instantiate(inst);
8874-
return CORINFO_CLASS_HANDLE(resultTh.GetMethodTable());
8875-
}
8847+
// If T implements IComparable<T>
8848+
if (elemTypeHnd.CanCastTo(TypeHandle(CoreLibBinder::GetClass(CLASS__ICOMPARABLEGENERIC)).Instantiate(inst)))
8849+
{
8850+
TypeHandle resultTh = ((TypeHandle)CoreLibBinder::GetClass(CLASS__GENERIC_COMPARER)).Instantiate(inst);
8851+
return CORINFO_CLASS_HANDLE(resultTh.GetMethodTable());
88768852
}
88778853

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

8916-
// Special case for byte
8917-
if (elemTypeHnd.AsMethodTable()->HasSameTypeDefAs(CoreLibBinder::GetClass(CLASS__ELEMENT_TYPE_U1)))
8918-
{
8919-
return CORINFO_CLASS_HANDLE(CoreLibBinder::GetClass(CLASS__BYTE_EQUALITYCOMPARER));
8920-
}
8921-
89228892
// Mirrors the logic in BCL's CompareHelpers.CreateDefaultComparer
89238893
// And in compile.cpp's SpecializeComparer
89248894
//
89258895
// We need to find the appropriate instantiation
89268896
Instantiation inst(&elemTypeHnd, 1);
89278897

8928-
// If T implements IEquatable<T>
8929-
if (elemTypeHnd.CanCastTo(TypeHandle(CoreLibBinder::GetClass(CLASS__IEQUATABLEGENERIC)).Instantiate(inst)))
8930-
{
8931-
TypeHandle resultTh = ((TypeHandle)CoreLibBinder::GetClass(CLASS__GENERIC_EQUALITYCOMPARER)).Instantiate(inst);
8932-
return CORINFO_CLASS_HANDLE(resultTh.GetMethodTable());
8933-
}
8934-
89358898
// Nullable<T>
89368899
if (Nullable::IsNullableType(elemTypeHnd))
89378900
{
@@ -8946,33 +8909,20 @@ CORINFO_CLASS_HANDLE CEEInfo::getDefaultEqualityComparerClassHelper(CORINFO_CLAS
89468909
// to avoid boxing and call the correct versions of GetHashCode.
89478910
if (elemTypeHnd.IsEnum())
89488911
{
8949-
MethodTable* targetClass = NULL;
8950-
CorElementType normType = elemTypeHnd.GetVerifierCorElementType();
8951-
8952-
switch(normType)
8953-
{
8954-
case ELEMENT_TYPE_I1:
8955-
case ELEMENT_TYPE_I2:
8956-
case ELEMENT_TYPE_U1:
8957-
case ELEMENT_TYPE_U2:
8958-
case ELEMENT_TYPE_I4:
8959-
case ELEMENT_TYPE_U4:
8960-
case ELEMENT_TYPE_I8:
8961-
case ELEMENT_TYPE_U8:
8962-
{
8963-
targetClass = CoreLibBinder::GetClass(CLASS__ENUM_EQUALITYCOMPARER);
8964-
break;
8965-
}
8912+
TypeHandle resultTh = ((TypeHandle)CoreLibBinder::GetClass(CLASS__ENUM_EQUALITYCOMPARER)).Instantiate(inst);
8913+
return CORINFO_CLASS_HANDLE(resultTh.GetMethodTable());
8914+
}
89668915

8967-
default:
8968-
break;
8969-
}
8916+
if (elemTypeHnd.IsCanonicalSubtype())
8917+
{
8918+
return NULL;
8919+
}
89708920

8971-
if (targetClass != NULL)
8972-
{
8973-
TypeHandle resultTh = ((TypeHandle)targetClass->GetCanonicalMethodTable()).Instantiate(inst);
8974-
return CORINFO_CLASS_HANDLE(resultTh.GetMethodTable());
8975-
}
8921+
// If T implements IEquatable<T>
8922+
if (elemTypeHnd.CanCastTo(TypeHandle(CoreLibBinder::GetClass(CLASS__IEQUATABLEGENERIC)).Instantiate(inst)))
8923+
{
8924+
TypeHandle resultTh = ((TypeHandle)CoreLibBinder::GetClass(CLASS__GENERIC_EQUALITYCOMPARER)).Instantiate(inst);
8925+
return CORINFO_CLASS_HANDLE(resultTh.GetMethodTable());
89768926
}
89778927

89788928
// Default case

‎src/mono/System.Private.CoreLib/src/System/Collections/Generic/EqualityComparer.Mono.cs

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -35,11 +35,7 @@ private static EqualityComparer<T> CreateComparer()
3535
// IN mini_handle_call_res_devirt
3636
/////////////////////////////////////////////////
3737

38-
if (t == typeof(byte))
39-
{
40-
return (EqualityComparer<T>)(object)(new ByteEqualityComparer());
41-
}
42-
else if (t == typeof(string))
38+
if (t == typeof(string))
4339
{
4440
// Specialize for string, as EqualityComparer<string>.Default is on the startup path
4541
return (EqualityComparer<T>)(object)(new GenericEqualityComparer<string>());

‎src/mono/System.Private.CoreLib/src/System/Runtime/CompilerServices/JitHelpers.cs

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,13 +5,11 @@ namespace System.Runtime.CompilerServices
55
{
66
internal static class JitHelpers
77
{
8-
#pragma warning disable IDE0060
98
[Intrinsic]
10-
public static bool EnumEquals<T>(T x, T y) where T : struct, Enum => throw new NotImplementedException();
9+
public static bool EnumEquals<T>(T x, T y) where T : struct, Enum => x.Equals(y);
1110

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

1614
[Intrinsic]
1715
internal static void DisableInline () => throw new NotImplementedException();

‎src/mono/mono/mini/interp/transform.c

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2540,7 +2540,10 @@ interp_handle_intrinsics (TransformData *td, MonoMethod *target_method, MonoClas
25402540
MonoType *t = ctx->method_inst->type_argv [0];
25412541
t = mini_get_underlying_type (t);
25422542

2543-
gboolean is_i8 = (t->type == MONO_TYPE_I8 || t->type == MONO_TYPE_U8);
2543+
if (t->type == MONO_TYPE_R4 || t->type == MONO_TYPE_R8)
2544+
return FALSE;
2545+
2546+
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)));
25442547
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);
25452548

25462549
gboolean is_compareto = strcmp (tm, "EnumCompareTo") == 0;

‎src/mono/mono/mini/intrinsics.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -672,10 +672,10 @@ emit_jit_helpers_intrinsics (MonoCompile *cfg, MonoMethod *cmethod, MonoMethodSi
672672

673673
t = ctx->method_inst->type_argv [0];
674674
t = mini_get_underlying_type (t);
675-
if (mini_is_gsharedvt_variable_type (t))
675+
if (mini_is_gsharedvt_variable_type (t) || t->type == MONO_TYPE_R4 || t->type == MONO_TYPE_R8)
676676
return NULL;
677677

678-
gboolean is_i8 = (t->type == MONO_TYPE_I8 || t->type == MONO_TYPE_U8);
678+
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)));
679679
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);
680680
int cmp_op, ceq_op, cgt_op, clt_op;
681681

‎src/mono/mono/mini/mini.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4301,7 +4301,7 @@ mini_handle_call_res_devirt (MonoMethod *cmethod)
43014301

43024302
// EqualityComparer<T>.Default returns specific types depending on T
43034303
// FIXME: Special case more types: byte, string, nullable, enum ?
4304-
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) {
4304+
if (mono_class_is_assignable_from_internal (inst, mono_class_from_mono_type_internal (param_type)) && param_type->type != MONO_TYPE_STRING) {
43054305
MonoClass *gcomparer_inst;
43064306

43074307
memset (&ctx, 0, sizeof (ctx));

0 commit comments

Comments
 (0)