Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert RuntimeHelpers intrinsics to JIT ones #88543

Closed
wants to merge 9 commits into from
Closed

Convert RuntimeHelpers intrinsics to JIT ones #88543

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
2bfc816
Convert RuntimeHelpers intrinsics to JIT ones
MichalPetryka Jul 8, 2023
4a43af0
Apply suggestions from code review
MichalPetryka Jul 8, 2023
c6db768
Fix formatting, mono and smalltypes
MichalPetryka Jul 8, 2023
2b34801
Update importercalls.cpp
MichalPetryka Jul 8, 2023
5d01074
Fix mono
MichalPetryka Jul 8, 2023
24f2cf9
Really fix mono
MichalPetryka Jul 8, 2023
fcdf181
Try to fix asserts
MichalPetryka Jul 9, 2023
13bea8f
Fix flag setting
MichalPetryka Jul 9, 2023
5c8b92b
Try to fix the assert
MichalPetryka Jul 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 0 additions & 8 deletions src/coreclr/System.Private.CoreLib/src/System/Collections/Generic/Comparer.CoreCLR.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,4 @@ public abstract partial class Comparer<T> : IComparer, IComparer<T>
// as possible and define most of the creation logic in a non-generic class.
public static Comparer<T> Default { [Intrinsic] get; } = (Comparer<T>)ComparerHelpers.CreateDefaultComparer(typeof(T));
}

internal sealed partial class EnumComparer<T> : Comparer<T> where T : struct, Enum
{
public override int Compare(T x, T y)
{
return RuntimeHelpers.EnumCompareTo(x, y);
}
}
}
6 changes: 0 additions & 6 deletions ...coreclr/System.Private.CoreLib/src/System/Collections/Generic/EqualityComparer.CoreCLR.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,12 +161,6 @@ internal override int LastIndexOf(byte[] array, byte value, int startIndex, int

public sealed partial class EnumEqualityComparer<T> : EqualityComparer<T> where T : struct, Enum
{
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public override bool Equals(T x, T y)
{
return RuntimeHelpers.EnumEquals(x, y);
}

internal override int IndexOf(T[] array, T value, int startIndex, int count)
{
int endIndex = startIndex + count;
Expand Down
45 changes: 1 addition & 44 deletions ...eclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.CoreCLR.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,43 +188,6 @@ public static object GetUninitializedObject(
[MethodImpl(MethodImplOptions.InternalCall)]
internal static extern object AllocateUninitializedClone(object obj);

/// <returns>true if given type is reference type or value type that contains references</returns>
[Intrinsic]
public static bool IsReferenceOrContainsReferences<T>()
{
// The body of this function will be replaced by the EE with unsafe code!!!
// See getILIntrinsicImplementationForRuntimeHelpers for how this happens.
throw new InvalidOperationException();
}

/// <returns>true if given type is bitwise equatable (memcmp can be used for equality checking)</returns>
/// <remarks>
/// Only use the result of this for Equals() comparison, not for CompareTo() comparison.
/// </remarks>
[Intrinsic]
internal static bool IsBitwiseEquatable<T>()
{
// The body of this function will be replaced by the EE with unsafe code!!!
// See getILIntrinsicImplementationForRuntimeHelpers for how this happens.
throw new InvalidOperationException();
}

[Intrinsic]
internal static bool EnumEquals<T>(T x, T y) where T : struct, Enum
{
// The body of this function will be replaced by the EE with unsafe code
// See getILIntrinsicImplementation for how this happens.
return x.Equals(y);
}

[Intrinsic]
internal static int EnumCompareTo<T>(T x, T y) where T : struct, Enum
{
// The body of this function will be replaced by the EE with unsafe code
// See getILIntrinsicImplementation for how this happens.
return x.CompareTo(y);
}

internal static ref byte GetRawData(this object obj) =>
ref Unsafe.As<RawData>(obj).Data;

Expand Down Expand Up @@ -297,18 +260,12 @@ internal static unsafe bool ObjectHasComponentSize(object obj)
// ... work with pMT ...
//
// GC.KeepAlive(o);
//
[MethodImpl(MethodImplOptions.AggressiveInlining)]
[Intrinsic]
internal static unsafe MethodTable* GetMethodTable(object obj)
{
// The body of this function will be replaced by the EE with unsafe code
// See getILIntrinsicImplementationForRuntimeHelpers for how this happens.

return (MethodTable *)Unsafe.Add(ref Unsafe.As<byte, IntPtr>(ref obj.GetRawData()), -1);
return GetMethodTable(obj);
}


[LibraryImport(QCall, EntryPoint = "MethodTable_AreTypesEquivalent")]
[return: MarshalAs(UnmanagedType.Bool)]
internal static unsafe partial bool AreTypesEquivalent(MethodTable* pMTa, MethodTable* pMTb);
Expand Down
3 changes: 3 additions & 0 deletions src/coreclr/inc/corinfo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2354,6 +2354,9 @@ class ICorStaticInfo
// Quick check whether the type is a value class. Returns the same value as getClassAttribs(cls) & CORINFO_FLG_VALUECLASS, except faster.
virtual bool isValueClass(CORINFO_CLASS_HANDLE cls) = 0;

// Checks if type can be compared for equality as bytes.
virtual bool isBitwiseEquatable(CORINFO_CLASS_HANDLE cls) = 0;
Copy link
Member

@jkotas jkotas Jul 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have mixed feeling about introducing a new JIT/EE interface method just to support an expansion of one-off BCL method in the JIT. Providing the expansion as IL on the VM is less spread through the system.

This direction makes sense only if we believe that 100% of the IL intrinsics are going to move to the JIT eventually. We would need to have a number of new one-off methods like this to pull it off.

@dotnet/jit-contrib Opinions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say that the this proper recognition of this intrinsic as a constant in the JIT is important since it makes sure it can properly skip importing branches guarded by it, similar to how IsKnownConstant and IsReferenceOrContainsReferences are used. It's also tracked for exposal in #75642.

As far as I could see, the only IL intrinsics left in the CoreCLR VM are the Unsafe ones (tracked for removal in #69220), Interlocked.CompareExchange(T) which can be removed with no impact today since it already has an equivalent managed implementation with Unsafe.As and Activator.CreateInstance<T> which seems to be the only problematic one.

Since "bitwise equality" is a somewhat fundamental type property (and could potentially be useful for other optimizations in the JIT), I've considered making this a flag in getClassAttribs, but I've refrained from doing so since it's I think relatively expensive to fetch and wouldn't be used that often.
However, citing @SingleAccretion from Discord:

As Tanner notes, it may be good to design a more general mechanism to ask questions about facts like that.
For example, an API that takes flags with the "requests" and returns answers to only the questions asked.

maybe it'd make sense to introduce getExtendedClassAttribs(uint32_t mask) that'd only fetch more rarely used/expensive flags (with a new separate enum for them) like this or #88136 based on the mask?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This direction makes sense only if we believe that 100% of the IL intrinsics are going to move to the JIT eventually

Do we have a list of the APIs that would still require moving and how complex they are to handle? That is, are many of them effectively simple flags like this one which are often trivial to lookup and which may get used on generic hot paths or do we have a number of substantially more complex APIs where the benefits of special-casing them in the JIT are more questionable?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it can properly skip importing branches guarded by it, similar to how IsKnownConstant and IsReferenceOrContainsReferences are used

FWIW, this is a workaround for a deficiency in the current JIT inlining strategy. It would be better to fix it in more general way for everybody out there instead of using it to justify implementing more methods as JIT intrinsics.

Interlocked.CompareExchange(T) which can be removed with no impact today

I expect that we would see performance regressions in shared generic code callsites. It is a workaround for the current shared generic code inlining limitation.

maybe it'd make sense to introduce getExtendedClassAttribs(uint32_t mask)

getExtendedClassAttribs(uint32_t mask) would make sense if the typically caller needs most of the flags and when there is an efficiency benefit from computing and returning multiple flags together. If the typically caller needs just one flag as is the case for expanding one-off Intrinsics, it is better to have an API that returns the one property directly and avoids the extra level of abstraction.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a list of the APIs that would still require moving and how complex they are to handle?

The current CoreCLR and native AOT intrinsics that would required introducing new special new JIT-EE interface APIs. You can generally assume that it is one new JIT/EE interface method for each:

  • Activaror.CreateInstanceOf<T>
  • EqualityCompaparer/Comparer.Create<T>
  • EqualityComparerHelpers.GetComparerForReferenceTypesOnly/StructOnlyEquals
  • Interlocked.CompareExchange<T>(ref)
  • RuntimeAugments.GetCanonType
  • MethodTable.SupportsRelativePointers
  • System.IO.Stream.HasOverriddenBeginEndRead/HasOverriddenBeginEndReadHasOverriddenBeginEndWrite

We like introducing intrinsics to do various tricks, so this list would grow over time. Also, some of these would require special-handling in the native AOT scanner if they are not IL intrinsics anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, this is a workaround for a deficiency in the current JIT inlining strategy. It would be better to fix it in more general way for everybody out there instead of using it to justify implementing more methods as JIT intrinsics.

Even then, having just a temporary workaround is better than not having one.

I expect that we would see performance regressions in shared generic code callsites. It is a workaround for the current shared generic code inlining limitation.

I don't really think that's the case, I think the only difference is the smaller IL size due to no Unsafe.As calls and the inlining boost from being an intrinsic.

runtime/src/coreclr/System.Private.CoreLib/src/System/Threading/Interlocked.CoreCLR.cs

Lines 120 to 128 in 3110693

// Note that getILIntrinsicImplementationForInterlocked() in vm\jitinterface.cpp replaces
// the body of the following method with the following IL:
// ldarg.0
// ldarg.1
// ldarg.2
// call System.Threading.Interlocked::CompareExchange(ref Object, Object, Object)
// ret
// The workaround is no longer strictly necessary now that we have Unsafe.As but it does
// have the advantage of being less sensitive to JIT's inliner decisions.

The current CoreCLR and native AOT intrinsics that would required introducing new special new JIT-EE interface APIs. You can generally assume that it is one new JIT/EE interface method for each

I think that it only makes sense to move apis that are: performance sensitive (like the three Is APIs here), more valid (like the GetMethodTable one) or ones that have big implementations in both runtimes (like the enum ones). I don't think it makes much sense to have runtime specific, rarely used intrinsics like HasOverriddenBeginEndRead that'd just call into the VM.

Copy link
Member

@jkotas jkotas Jul 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

having just a temporary workaround is better than not having one.

Can you come up with a real-world looking example where isBitwiseEquatable expansion in the JIT makes a difference to show that it is worth doing?

more valid (like the GetMethodTable one)

Yes, the GetMethodTable one looks good.

ones that have big implementations in both runtimes (like the enum ones)

The implementations in each runtime are about 30 lines of code each. I would not call that big. The inlining expansion in the JIT is not reducing runtime complexity much overall.

The expansion in the JIT makes the code less maintainable by creating a copy of CompareTo implementation from 10 different .cs files into the JIT. To make this explicit, you would want to add a comment "This logic is duplicated in the JIT in EnumCompare intrinsic expansion" to CompareTo implementation in all Byte.cs, UByte.cs, ...., etc. I do not think it would be a good idea.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interlocked.CompareExchange(T) which can be removed with no impact today

I don't really think that's the case, I think the only difference is the smaller IL size due to no Unsafe.As calls and the inlining boost from being an intrinsic.

I don't really think that's the case, I think the only difference is the smaller IL size due to no Unsafe.As calls and the inlining boost from being an intrinsic.

I have done a few quick tests and I agree that the IL intrinsic for Interlocked.CompareExchange does not seem to be making a difference. Feel free to remove it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dotnet/jit-contrib Opinions?

I don't understand the motivation for these changes. It seems like a lot of code churn late in a release cycle with a decent risk of introducing correctness or perf issues.

If we think further unification / simplification here is sufficient motivation, perhaps it can wait until .NET 9?


// Decides how the JIT should do the optimization to inline the check for
// GetTypeFromHandle(handle) == obj.GetType() (for CORINFO_INLINE_TYPECHECK_SOURCE_VTABLE)
// GetTypeFromHandle(X) == GetTypeFromHandle(Y) (for CORINFO_INLINE_TYPECHECK_SOURCE_TOKEN)
Expand Down
3 changes: 3 additions & 0 deletions src/coreclr/inc/icorjitinfoimpl_generated.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,9 @@ size_t printClassName(
bool isValueClass(
CORINFO_CLASS_HANDLE cls) override;

bool isBitwiseEquatable(
CORINFO_CLASS_HANDLE cls) override;

CorInfoInlineTypeCheck canInlineTypeCheck(
CORINFO_CLASS_HANDLE cls,
CorInfoInlineTypeCheckSource source) override;
Expand Down
10 changes: 5 additions & 5 deletions src/coreclr/inc/jiteeversionguid.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,11 @@ typedef const GUID *LPCGUID;
#define GUID_DEFINED
#endif // !GUID_DEFINED

constexpr GUID JITEEVersionIdentifier = { /* 02e334af-4e6e-4a68-9feb-308d3d2661bc */
0x2e334af,
0x4e6e,
0x4a68,
{0x9f, 0xeb, 0x30, 0x8d, 0x3d, 0x26, 0x61, 0xbc}
constexpr GUID JITEEVersionIdentifier = { /* f81c24a8-c7c1-420c-859c-36183e237c02 */
0xf81c24a8,
0xc7c1,
0x420c,
{0x85, 0x9c, 0x36, 0x18, 0x3e, 0x23, 0x7c, 0x02}
};

//////////////////////////////////////////////////////////////////////////////////////////////////////////
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/jit/ICorJitInfo_names_generated.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ DEF_CLR_API(getClassNameFromMetadata)
DEF_CLR_API(getTypeInstantiationArgument)
DEF_CLR_API(printClassName)
DEF_CLR_API(isValueClass)
DEF_CLR_API(isBitwiseEquatable)
DEF_CLR_API(canInlineTypeCheck)
DEF_CLR_API(getClassAttribs)
DEF_CLR_API(getClassModule)
Expand Down
9 changes: 9 additions & 0 deletions src/coreclr/jit/ICorJitInfo_wrapper_generated.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,6 +387,15 @@ bool WrapICorJitInfo::isValueClass(
return temp;
}

bool WrapICorJitInfo::isBitwiseEquatable(
CORINFO_CLASS_HANDLE cls)
{
API_ENTER(isBitwiseEquatable);
bool temp = wrapHnd->isBitwiseEquatable(cls);
API_LEAVE(isBitwiseEquatable);
return temp;
}

CorInfoInlineTypeCheck WrapICorJitInfo::canInlineTypeCheck(
CORINFO_CLASS_HANDLE cls,
CorInfoInlineTypeCheckSource source)
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/jit/compiler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3904,6 +3904,7 @@ class Compiler
void impImportLeave(BasicBlock* block);
void impResetLeaveBlock(BasicBlock* block, unsigned jmpAddr);
GenTree* impTypeIsAssignable(GenTree* typeTo, GenTree* typeFrom);
GenTree* impImportCompare(GenTree* op1, GenTree* op2, genTreeOps oper, bool isUnsignedOrUnordered);

// Mirrors StringComparison.cs
enum StringComparison
Expand Down
85 changes: 44 additions & 41 deletions src/coreclr/jit/importer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2424,6 +2424,49 @@ GenTree* Compiler::impTypeIsAssignable(GenTree* typeTo, GenTree* typeFrom)
return nullptr;
}

GenTree* Compiler::impImportCompare(GenTree* op1, GenTree* op2, genTreeOps oper, bool isUnsignedOrUnordered)
{
// Recognize the IL idiom of CGT_UN(op1, 0) and normalize
// it so that downstream optimizations don't have to.
if ((oper == GT_GT) && isUnsignedOrUnordered && op2->IsIntegralConst(0))
{
oper = GT_NE;
isUnsignedOrUnordered = false;
}

#ifdef TARGET_64BIT
if (varTypeIsI(op1) && genActualTypeIsInt(op2))
{
op2 = impImplicitIorI4Cast(op2, TYP_I_IMPL);
}
else if (varTypeIsI(op2) && genActualTypeIsInt(op1))
{
op1 = impImplicitIorI4Cast(op1, TYP_I_IMPL);
}
#endif // TARGET_64BIT

assert(genActualType(op1) == genActualType(op2) || (varTypeIsI(op1) && varTypeIsI(op2)) ||
(varTypeIsFloating(op1) && varTypeIsFloating(op2)));

if ((op1->TypeGet() != op2->TypeGet()) && varTypeIsFloating(op1))
{
op1 = impImplicitR4orR8Cast(op1, TYP_DOUBLE);
op2 = impImplicitR4orR8Cast(op2, TYP_DOUBLE);
}

// Create the comparison node.
op1 = gtNewOperNode(oper, TYP_INT, op1, op2);

// TODO: setting both flags when only one is appropriate.
if (isUnsignedOrUnordered)
{
op1->gtFlags |= GTF_RELOP_NAN_UN | GTF_UNSIGNED;
}

// Fold result, if possible.
return gtFoldExpr(op1);
}

/*****************************************************************************
* 'logMsg' is true if a log message needs to be logged. false if the caller has
* already logged it (presumably in a more detailed fashion than done here)
Expand Down Expand Up @@ -7396,47 +7439,7 @@ void Compiler::impImportBlockCode(BasicBlock* block)
op2 = impPopStack().val;
op1 = impPopStack().val;

// Recognize the IL idiom of CGT_UN(op1, 0) and normalize
// it so that downstream optimizations don't have to.
if ((opcode == CEE_CGT_UN) && op2->IsIntegralConst(0))
{
oper = GT_NE;
uns = false;
}

#ifdef TARGET_64BIT
if (varTypeIsI(op1) && genActualTypeIsInt(op2))
{
op2 = impImplicitIorI4Cast(op2, TYP_I_IMPL);
}
else if (varTypeIsI(op2) && genActualTypeIsInt(op1))
{
op1 = impImplicitIorI4Cast(op1, TYP_I_IMPL);
}
#endif // TARGET_64BIT

assertImp(genActualType(op1) == genActualType(op2) || (varTypeIsI(op1) && varTypeIsI(op2)) ||
(varTypeIsFloating(op1) && varTypeIsFloating(op2)));

if ((op1->TypeGet() != op2->TypeGet()) && varTypeIsFloating(op1))
{
op1 = impImplicitR4orR8Cast(op1, TYP_DOUBLE);
op2 = impImplicitR4orR8Cast(op2, TYP_DOUBLE);
}

// Create the comparison node.
op1 = gtNewOperNode(oper, TYP_INT, op1, op2);

// TODO: setting both flags when only one is appropriate.
if (uns)
{
op1->gtFlags |= GTF_RELOP_NAN_UN | GTF_UNSIGNED;
}

// Fold result, if possible.
op1 = gtFoldExpr(op1);

impPushOnStack(op1, tiRetVal);
impPushOnStack(impImportCompare(op1, op2, oper, uns), tiRetVal);
break;

case CEE_BEQ_S:
Expand Down
102 changes: 102 additions & 0 deletions src/coreclr/jit/importercalls.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2551,6 +2551,9 @@ GenTree* Compiler::impIntrinsic(GenTree* newobjThis,
// This one is just `return true/false`
case NI_System_Runtime_CompilerServices_RuntimeHelpers_IsKnownConstant:

case NI_System_Runtime_CompilerServices_RuntimeHelpers_EnumEquals:
case NI_System_Runtime_CompilerServices_RuntimeHelpers_EnumCompareTo:

// We need these to be able to fold "typeof(...) == typeof(...)"
case NI_System_Type_GetTypeFromHandle:
case NI_System_Type_op_Equality:
Expand Down Expand Up @@ -2759,6 +2762,81 @@ GenTree* Compiler::impIntrinsic(GenTree* newobjThis,
break;
}

case NI_System_Runtime_CompilerServices_RuntimeHelpers_IsReference:
{
assert(sig->sigInst.methInstCount == 1);
CORINFO_CLASS_HANDLE hClass = sig->sigInst.methInst[0];
retNode = gtNewIconNode(eeIsValueClass(hClass) ? 0 : 1);
break;
}

case NI_System_Runtime_CompilerServices_RuntimeHelpers_IsReferenceOrContainsReferences:
{
assert(sig->sigInst.methInstCount == 1);
CORINFO_CLASS_HANDLE hClass = sig->sigInst.methInst[0];
retNode = gtNewIconNode(!eeIsValueClass(hClass) || ((info.compCompHnd->getClassAttribs(hClass) & CORINFO_FLG_CONTAINS_GC_PTR) != 0) ? 1 : 0);
break;
}

case NI_System_Runtime_CompilerServices_RuntimeHelpers_IsBitwiseEquatable:
{
assert(sig->sigInst.methInstCount == 1);
CORINFO_CLASS_HANDLE hClass = sig->sigInst.methInst[0];
retNode = gtNewIconNode(info.compCompHnd->isBitwiseEquatable(hClass) ? 1 : 0);
break;
}

case NI_System_Runtime_CompilerServices_RuntimeHelpers_GetMethodTable:
{
retNode = gtNewMethodTableLookup(impPopStack().val);
jkotas marked this conversation as resolved.
Show resolved Hide resolved
break;
}

case NI_System_Runtime_CompilerServices_RuntimeHelpers_EnumEquals:
{
assert(sig->sigInst.methInstCount == 1);
CORINFO_CLASS_HANDLE hClass = sig->sigInst.methInst[0];
CorInfoType jitType = info.compCompHnd->asCorInfoType(hClass);

if (varTypeIsFloating(JitType2PreciseVarType(jitType)))
{
return nullptr;
}

GenTree* op2 = impPopStack().val;
GenTree* op1 = impPopStack().val;
retNode = impImportCompare(op1, op2, GT_EQ, false);
break;
}

case NI_System_Runtime_CompilerServices_RuntimeHelpers_EnumCompareTo:
{
assert(sig->sigInst.methInstCount == 1);

CORINFO_CLASS_HANDLE hClass = sig->sigInst.methInst[0];
CorInfoType jitType = info.compCompHnd->asCorInfoType(hClass);
var_types type = JitType2PreciseVarType(jitType);

if (varTypeIsFloating(type))
{
return nullptr;
}

GenTree* op2 = impPopStack().val;
GenTree* op1 = impPopStack().val;
bool uns = varTypeIsUnsigned(type);

GenTree* op1Clone;
GenTree* op2Clone;
op1 = impCloneExpr(op1, &op1Clone, CHECK_SPILL_ALL, nullptr DEBUGARG("EnumCompareTo arg1"));
Copy link
Member

@jkotas jkotas Jul 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is less efficient that the existing implementation for 8-bit and 16-bit enums.

Also, this may be better marked as mustExpand so that there are no observable behavior differences between the expanded and non-expanded version.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existing implementation that just called CompareTo on the underlying type feels more robust, less duplicative.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is less efficient that the existing implementation for 8-bit and 16-bit enums.

Fixed.

Also, this may be better marked as mustExpand so that there are no observable behavior differences between the expanded and non-expanded version.

It's marked as betterExpand now, which means that it'll expand unless in debug AFAIR. I don't think we should have any behaviour differences between the two paths.

The existing implementation that just called CompareTo on the underlying type feels more robust, less duplicative.

But it also relies on that hardcoded table of tables for ILs for selected types there and it relies more on the inliner. This logic resembles the Mono one where it's expanded directly in the intrinsic too. We could potentially change this to PrimiviteCompareTo and use it for all primitives in their CompareTos or we could just remove the intrinsic and rewrite the code in managed with the Enum.GetUnderlyingType intrinsic instead (but that could potentially run into inlining issues due to IL size).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's marked as betterExpand now, which means that it'll expand unless in debug AFAIR.

It means that the debug code will allocate vs. release code will not. The performance difference between debug and release is going to be order of magnitude more than what it is today. Performance regressions like this are not acceptable for debug code. It is not unusual to see issues filled on large debug code performance regressions like this by our customers.

relies on that hardcoded table of tables for ILs for selected types there

It is not hard to fix the code so that it works for all enum underlying types.

it relies more on the inliner

The method being inlined is very small. There are thousands of places in this repo where we have small methods to make the code easier to maintain and depend on them being inlined without an issue. Why should this one be different? Enum comparers are not that common for inliner avoidance to make difference.

we could just remove the intrinsic and rewrite the code in managed with the Enum.GetUnderlyingType intrinsic instead (but that could potentially run into inlining issues due to IL size).

Right, this approach would not work well.

op2 = impCloneExpr(op2, &op2Clone, CHECK_SPILL_ALL, nullptr DEBUGARG("EnumCompareTo arg2"));

retNode = gtFoldExpr(gtNewOperNode(GT_SUB, TYP_INT,
impImportCompare(op1, op2, GT_GT, uns),
impImportCompare(op1Clone, op2Clone, GT_LT, uns)));
break;
}

case NI_System_Runtime_InteropService_MemoryMarshal_GetArrayDataReference:
{
assert(sig->numArgs == 1);
Expand Down Expand Up @@ -8948,6 +9026,30 @@ NamedIntrinsic Compiler::lookupNamedIntrinsic(CORINFO_METHOD_HANDLE method)
{
result = NI_System_Runtime_CompilerServices_RuntimeHelpers_IsKnownConstant;
}
else if (strcmp(methodName, "IsReference") == 0)
{
result = NI_System_Runtime_CompilerServices_RuntimeHelpers_IsReference;
}
else if (strcmp(methodName, "IsReferenceOrContainsReferences") == 0)
{
result = NI_System_Runtime_CompilerServices_RuntimeHelpers_IsReferenceOrContainsReferences;
}
else if (strcmp(methodName, "IsBitwiseEquatable") == 0)
{
result = NI_System_Runtime_CompilerServices_RuntimeHelpers_IsBitwiseEquatable;
}
else if (strcmp(methodName, "EnumEquals") == 0)
{
result = NI_System_Runtime_CompilerServices_RuntimeHelpers_EnumEquals;
}
else if (strcmp(methodName, "EnumCompareTo") == 0)
{
result = NI_System_Runtime_CompilerServices_RuntimeHelpers_EnumCompareTo;
}
else if (strcmp(methodName, "GetMethodTable") == 0)
{
result = NI_System_Runtime_CompilerServices_RuntimeHelpers_GetMethodTable;
}
}
else if (strcmp(className, "Unsafe") == 0)
{
Expand Down
6 changes: 6 additions & 0 deletions src/coreclr/jit/namedintrinsiclist.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,12 @@ enum NamedIntrinsic : unsigned short
NI_System_Runtime_CompilerServices_RuntimeHelpers_CreateSpan,
NI_System_Runtime_CompilerServices_RuntimeHelpers_InitializeArray,
NI_System_Runtime_CompilerServices_RuntimeHelpers_IsKnownConstant,
NI_System_Runtime_CompilerServices_RuntimeHelpers_IsReference,
NI_System_Runtime_CompilerServices_RuntimeHelpers_IsReferenceOrContainsReferences,
NI_System_Runtime_CompilerServices_RuntimeHelpers_IsBitwiseEquatable,
NI_System_Runtime_CompilerServices_RuntimeHelpers_EnumEquals,
NI_System_Runtime_CompilerServices_RuntimeHelpers_EnumCompareTo,
NI_System_Runtime_CompilerServices_RuntimeHelpers_GetMethodTable,

NI_System_Runtime_InteropService_MemoryMarshal_GetArrayDataReference,

Expand Down
Loading