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.

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

Fold always false type checks #99761

Merged
merged 14 commits into from
Mar 18, 2024
3 changes: 3 additions & 0 deletions src/coreclr/inc/corinfo.h
Original file line number Diff line number Diff line change
Expand Up @@ -1247,6 +1247,7 @@ struct CORINFO_LOOKUP_KIND
#define CORINFO_USEHELPER ((uint16_t) 0xffff)
#define CORINFO_USENULL ((uint16_t) 0xfffe)
#define CORINFO_NO_SIZE_CHECK ((uint16_t) 0xffff)
#define CORINFO_NEVER_INSTANTIATED (-1)

struct CORINFO_RUNTIME_LOOKUP
{
Expand Down Expand Up @@ -2983,6 +2984,8 @@ class ICorStaticInfo
// Obtains a list of exact classes for a given base type. Returns 0 if the number of
// the exact classes is greater than maxExactClasses or if more types might be loaded
// in future.
// When called with maxExactClasses == 0, returns CORINFO_NEVER_INSTANTIATED if an
// exact class of this type cannot exist. Returns 0 otherwise.
virtual int getExactClasses(
MichalStrehovsky marked this conversation as resolved.
Show resolved Hide resolved
CORINFO_CLASS_HANDLE baseType, /* IN */
int maxExactClasses, /* IN */
Expand Down
31 changes: 24 additions & 7 deletions src/coreclr/jit/gentree.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -14050,13 +14050,8 @@ GenTree* Compiler::gtFoldTypeCompare(GenTree* tree)
return compare;
}

// If one operand creates a type from a handle and the other operand is fetching the type from an object,
// we can sometimes optimize the type compare into a simpler
// method table comparison.
//
// TODO: if other operand is null...
if (!(((op1Kind == TPK_GetType) && (op2Kind == TPK_Handle)) ||
((op1Kind == TPK_Handle) && (op2Kind == TPK_GetType))))
// In the following code, we need one of these to be a handle.
if (op1Kind != TPK_Handle && op2Kind != TPK_Handle)
{
return tree;
}
Expand All @@ -14074,6 +14069,28 @@ GenTree* Compiler::gtFoldTypeCompare(GenTree* tree)
return tree;
}

// Check if an object of this type can even exist
if (info.compCompHnd->getExactClasses(clsHnd, 0, nullptr) == CORINFO_NEVER_INSTANTIATED)
{
JITDUMP("Runtime reported %p (%s) is never allocated\n", dspPtr(clsHnd), eeGetClassName(clsHnd));

const bool operatorIsEQ = (oper == GT_EQ);
const int compareResult = operatorIsEQ ? 0 : 1;
JITDUMP("Runtime reports comparison is known at jit time: %u\n", compareResult);
GenTree* result = gtNewIconNode(compareResult);
return result;
}

// If one operand creates a type from a handle and the other operand is fetching the type from an object,
// we can sometimes optimize the type compare into a simpler
// method table comparison.
//
// TODO: if other operand is null...
if (op1Kind != TPK_GetType && op2Kind != TPK_GetType)
{
return tree;
}

// We're good to go.
JITDUMP("Optimizing compare of obj.GetType()"
" and type-from-handle to compare method table pointer\n");
Expand Down
27 changes: 26 additions & 1 deletion src/coreclr/jit/importer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5349,14 +5349,39 @@ GenTree* Compiler::impOptimizeCastClassOrIsInst(GenTree* op1, CORINFO_RESOLVED_T
return nullptr;
}

CORINFO_CLASS_HANDLE toClass = pResolvedToken->hClass;
if (info.compCompHnd->getExactClasses(toClass, 0, nullptr) == CORINFO_NEVER_INSTANTIATED)
{
JITDUMP("\nClass %s%p (%s) can never be allocated\n", dspPtr(toClass), eeGetClassName(toClass));
MichalStrehovsky marked this conversation as resolved.
Show resolved Hide resolved

if (!isCastClass)
{
JITDUMP("Cast will fail, optimizing to return null\n");

// If the cast was fed by a box, we can remove that too.
if (op1->IsBoxedValue())
{
JITDUMP("Also removing upstream box\n");
gtTryRemoveBoxUpstreamEffects(op1);
}

if (gtTreeHasSideEffects(op1, GTF_SIDE_EFFECT))
{
impAppendTree(op1, CHECK_SPILL_ALL, impCurStmtDI);
}
return gtNewNull();
}

JITDUMP("Cast will always throw, but not optimizing yet\n");
}

// See what we know about the type of the object being cast.
bool isExact = false;
bool isNonNull = false;
CORINFO_CLASS_HANDLE fromClass = gtGetClassHandle(op1, &isExact, &isNonNull);

if (fromClass != nullptr)
{
CORINFO_CLASS_HANDLE toClass = pResolvedToken->hClass;
JITDUMP("\nConsidering optimization of %s from %s%p (%s) to %p (%s)\n", isCastClass ? "castclass" : "isinst",
isExact ? "exact " : "", dspPtr(fromClass), eeGetClassName(fromClass), dspPtr(toClass),
eeGetClassName(toClass));
Expand Down
16 changes: 10 additions & 6 deletions src/coreclr/tools/Common/Compiler/DevirtualizationManager.cs
Original file line number Diff line number Diff line change
Expand Up @@ -203,13 +203,17 @@ protected virtual MethodDesc ResolveVirtualMethod(MethodDesc declMethod, DefType

#if !READYTORUN
/// <summary>
/// Gets a value indicating whether it might be possible to obtain a constructed type data structure for the given type.
/// Gets a value indicating whether it might be possible to obtain a constructed type data structure for the given type
/// in this compilation (i.e. is it possible to reference a constructed MethodTable symbol for this).
/// </summary>
/// <remarks>
/// This is a bit of a hack, but devirtualization manager has a global view of all allocated types
/// so it can answer this question.
/// </remarks>
public virtual bool CanConstructType(TypeDesc type) => true;
public virtual bool CanReferenceConstructedMethodTable(TypeDesc type) => true;

/// <summary>
/// Gets a value indicating whether a (potentially canonically-equlivalent) constructed MethodTable could
/// exist. This is similar to <see cref="CanReferenceConstructedMethodTable"/>, but will return true
/// for List&lt;__Canon&gt; if a constructed MethodTable for List&lt;object&gt; exists.
/// </summary>
public virtual bool CanTypeOrCanonicalFormOfTypeBeAllocated(TypeDesc type) => true;

public virtual TypeDesc[] GetImplementingClasses(TypeDesc type) => null;
#endif
Expand Down
14 changes: 12 additions & 2 deletions src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2874,12 +2874,22 @@ private TypeCompareState compareTypesForEquality(CORINFO_CLASS_STRUCT_* cls1, CO
TypeDesc type1 = HandleToObject(cls1);
TypeDesc type2 = HandleToObject(cls2);

return TypeExtensions.CompareTypesForEquality(type1, type2) switch
TypeCompareState result = TypeExtensions.CompareTypesForEquality(type1, type2) switch
{
true => TypeCompareState.Must,
false => TypeCompareState.MustNot,
_ => TypeCompareState.May,
};

#if !READYTORUN
if (result == TypeCompareState.May
&& (canNeverHaveInstanceOfSubclassOf(type1) || canNeverHaveInstanceOfSubclassOf(type2)))
Copy link
Member

Choose a reason for hiding this comment

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

Can compareTypesForEquality operate on types that are not allocated, but otherwise exist in the system? This change seems to make hidden assumptions about how this API is used by the JIT.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll think about this some more. Worst case I'll just pull this part out. I already used up my JitInterface update quota for the month.

Copy link
Member Author

Choose a reason for hiding this comment

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

It would not be a problem today but it adds a hidden assumption. Removed it. I'll measure how much it impacts things and whether we want to track that as a separate bug.

I know the typeof optimization would help CsWinRT but dont' have numbers.

Copy link
Member

Choose a reason for hiding this comment

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

We can fix that by adding a bool flag that says the query is specifically for System.Type operator==/!= and only execute this logic for that.

Copy link
Member Author

Choose a reason for hiding this comment

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

It would not be a problem today but it adds a hidden assumption. Removed it. I'll measure how much it impacts things and whether we want to track that as a separate bug.

I removed the wrong part. Fixed it in the latest iteration.

{
return TypeCompareState.MustNot;
}
#endif

return result;
}

private bool isMoreSpecificType(CORINFO_CLASS_STRUCT_* cls1, CORINFO_CLASS_STRUCT_* cls2)
Expand Down Expand Up @@ -4284,7 +4294,7 @@ private HRESULT getPgoInstrumentationResults(CORINFO_METHOD_STRUCT_* ftnHnd, ref
#pragma warning disable SA1001, SA1113, SA1115 // Commas should be spaced correctly
ComputeJitPgoInstrumentationSchema(ObjectToHandle, pgoResultsSchemas, out var nativeSchemas, _cachedMemoryStream
#if !READYTORUN
, _compilation.CanConstructType
, _compilation.CanReferenceConstructedMethodTable
#endif
);
#pragma warning restore SA1001, SA1113, SA1115 // Commas should be spaced correctly
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/tools/Common/JitInterface/CorInfoTypes.cs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ public static class CORINFO
public const ushort USEHELPER = 0xffff;
public const ushort USENULL = 0xfffe;
public const ushort CORINFO_NO_SIZE_CHECK = 0xffff;
public const int CORINFO_NEVER_INSTANTIATED = -1;
}

public struct CORINFO_METHOD_STRUCT_
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,9 +103,14 @@ public bool CanInline(MethodDesc caller, MethodDesc callee)
return _inliningPolicy.CanInline(caller, callee);
}

public bool CanConstructType(TypeDesc type)
public bool CanReferenceConstructedMethodTable(TypeDesc type)
{
return NodeFactory.DevirtualizationManager.CanConstructType(type);
return NodeFactory.DevirtualizationManager.CanReferenceConstructedMethodTable(type);
}

public bool CanTypeOrCanonicalFormOfTypeBeAllocated(TypeDesc type)
{
return NodeFactory.DevirtualizationManager.CanTypeOrCanonicalFormOfTypeBeAllocated(type);
}

public DelegateCreationInfo GetDelegateCtor(TypeDesc delegateType, MethodDesc target, TypeDesc constrainedType, bool followVirtualDispatch)
Expand Down Expand Up @@ -261,7 +266,7 @@ public bool NeedsRuntimeLookup(ReadyToRunHelperId lookupKind, object targetOfLoo

public ReadyToRunHelperId GetLdTokenHelperForType(TypeDesc type)
{
bool canConstructPerWholeProgramAnalysis = NodeFactory.DevirtualizationManager.CanConstructType(type);
bool canConstructPerWholeProgramAnalysis = NodeFactory.DevirtualizationManager.CanReferenceConstructedMethodTable(type);
bool creationAllowed = ConstructedEETypeNode.CreationAllowed(type);
return (canConstructPerWholeProgramAnalysis && creationAllowed)
? ReadyToRunHelperId.TypeHandle
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -413,7 +413,8 @@ public override DictionaryLayoutNode GetLayout(TypeSystemEntity methodOrType)

private sealed class ScannedDevirtualizationManager : DevirtualizationManager
{
private HashSet<TypeDesc> _constructedTypes = new HashSet<TypeDesc>();
private HashSet<TypeDesc> _constructedMethodTables = new HashSet<TypeDesc>();
private HashSet<TypeDesc> _canonConstructedMethodTables = new HashSet<TypeDesc>();
private HashSet<TypeDesc> _canonConstructedTypes = new HashSet<TypeDesc>();
private HashSet<TypeDesc> _unsealedTypes = new HashSet<TypeDesc>();
private Dictionary<TypeDesc, HashSet<TypeDesc>> _implementators = new();
Expand Down Expand Up @@ -442,7 +443,8 @@ public ScannedDevirtualizationManager(NodeFactory factory, ImmutableArray<Depend

if (type != null)
{
_constructedTypes.Add(type);
_constructedMethodTables.Add(type);
_canonConstructedMethodTables.Add(type.ConvertToCanonForm(CanonicalFormKind.Specific));
MichalStrehovsky marked this conversation as resolved.
Show resolved Hide resolved

if (type.IsInterface)
{
Expand Down Expand Up @@ -687,7 +689,11 @@ protected override MethodDesc ResolveVirtualMethod(MethodDesc declMethod, DefTyp
return result;
}

public override bool CanConstructType(TypeDesc type) => _constructedTypes.Contains(type);
public override bool CanReferenceConstructedMethodTable(TypeDesc type)
=> _constructedMethodTables.Contains(type);

public override bool CanTypeOrCanonicalFormOfTypeBeAllocated(TypeDesc type)
Copy link
Member

Choose a reason for hiding this comment

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

These two methods do very similar things, but they have very different names. Should these two methods have similar names, e.g. CanReferenceConstructedMethodTable + CanReferenceConstructedTypeOrCanonicalFormOfType; CanTypeBeAllocated + CanTypeOrCanonicalFormOfTypeBeAllocated ?

=> _constructedMethodTables.Contains(type) || _canonConstructedMethodTables.Contains(type);

public override TypeDesc[] GetImplementingClasses(TypeDesc type)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ public override IEETypeNode NecessaryTypeSymbolIfPossible(TypeDesc type)
// information proving that it isn't, give RyuJIT the constructed symbol even
// though we just need the unconstructed one.
// https://github.com/dotnet/runtimelab/issues/1128
bool canPotentiallyConstruct = NodeFactory.DevirtualizationManager.CanConstructType(type);
bool canPotentiallyConstruct = NodeFactory.DevirtualizationManager.CanReferenceConstructedMethodTable(type);
if (canPotentiallyConstruct)
return _nodeFactory.MaximallyConstructableType(type);

Expand All @@ -81,7 +81,7 @@ public override IEETypeNode NecessaryTypeSymbolIfPossible(TypeDesc type)

public FrozenRuntimeTypeNode NecessaryRuntimeTypeIfPossible(TypeDesc type)
{
bool canPotentiallyConstruct = NodeFactory.DevirtualizationManager.CanConstructType(type);
bool canPotentiallyConstruct = NodeFactory.DevirtualizationManager.CanReferenceConstructedMethodTable(type);
if (canPotentiallyConstruct)
return _nodeFactory.SerializedMaximallyConstructableRuntimeTypeObject(type);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2261,6 +2261,24 @@ private void getFieldInfo(ref CORINFO_RESOLVED_TOKEN pResolvedToken, CORINFO_MET
// and STS::AccessCheck::CanAccess.
}

private bool canNeverHaveInstanceOfSubclassOf(TypeDesc type)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
private bool canNeverHaveInstanceOfSubclassOf(TypeDesc type)
private bool CanNeverHaveInstanceOfSubclassOf(TypeDesc type)

Nit: We seem to use regular naming convention in this file for methods that are not directly exposed on JIT/EE interface

{
// Don't try to optimize nullable
if (type.IsNullable)
return false;

// We don't track unconstructable types very well and they are rare anyway
if (!ConstructedEETypeNode.CreationAllowed(type))
return false;

TypeDesc canonType = type.ConvertToCanonForm(CanonicalFormKind.Specific);

// If we don't have a constructed MethodTable for the exact type or for its template,
// this type or any of its subclasses can never be instantiated.
return !_compilation.CanTypeOrCanonicalFormOfTypeBeAllocated(type)
&& (type == canonType || !_compilation.CanReferenceConstructedMethodTable(canonType));
}

private int getExactClasses(CORINFO_CLASS_STRUCT_* baseType, int maxExactClasses, CORINFO_CLASS_STRUCT_** exactClsRet)
{
MetadataType type = HandleToObject(baseType) as MetadataType;
Expand All @@ -2269,6 +2287,14 @@ private int getExactClasses(CORINFO_CLASS_STRUCT_* baseType, int maxExactClasses
return 0;
}

if (maxExactClasses == 0)
{
if (canNeverHaveInstanceOfSubclassOf(type))
return CORINFO.CORINFO_NEVER_INSTANTIATED;

return 0;
}

// type is already sealed, return it
if (_compilation.IsEffectivelySealed(type))
{
Expand Down
Loading
Loading