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
Merged
2 changes: 1 addition & 1 deletion src/coreclr/inc/corinfo.h
Original file line number Diff line number Diff line change
Expand Up @@ -2983,7 +2983,7 @@ class ICorStaticInfo
CORINFO_CLASS_HANDLE* vcTypeRet /* OUT */
) = 0;

// Obtains a list of exact classes for a given base type. Returns 0 if the number of
// Obtains a list of exact classes for a given base type. Returns -1 if the number of
// the exact classes is greater than maxExactClasses or if more types might be loaded
// in future.
virtual int getExactClasses(
MichalStrehovsky marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
10 changes: 5 additions & 5 deletions src/coreclr/inc/jiteeversionguid.h
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 = { /* 6fd660c7-96be-4832-a84c-4200141f7d08 */
0x6fd660c7,
0x96be,
0x4832,
{0xa8, 0x4c, 0x42, 0x00, 0x14, 0x1f, 0x7d, 0x08}
constexpr GUID JITEEVersionIdentifier = { /* bdf34b26-0725-4ad6-9935-40bfd2a4c4fc */
0xbdf34b26,
0x0725,
0x4ad6,
{0x99, 0x35, 0x40, 0xbf, 0xd2, 0xa4, 0xc4, 0xfc}
};

//////////////////////////////////////////////////////////////////////////////////////////////////////////
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 @@ -5380,14 +5380,39 @@ GenTree* Compiler::impOptimizeCastClassOrIsInst(GenTree* op1, CORINFO_RESOLVED_T
return nullptr;
}

CORINFO_CLASS_HANDLE toClass = pResolvedToken->hClass;
if (info.compCompHnd->getExactClasses(toClass, 0, nullptr) == 0)
{
JITDUMP("\nClass %p (%s) can never be allocated\n", dspPtr(toClass), eeGetClassName(toClass));

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
2 changes: 1 addition & 1 deletion src/coreclr/jit/importercalls.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6588,7 +6588,7 @@ void Compiler::considerGuardedDevirtualization(GenTreeCall* call,
{
JITDUMP("No exact classes implementing %s\n", eeGetClassName(baseClass))
}
else if (numExactClasses > maxTypeChecks)
else if (numExactClasses < 0 || numExactClasses > maxTypeChecks)
{
JITDUMP("Too many exact classes implementing %s (%d > %d)\n", eeGetClassName(baseClass), numExactClasses,
maxTypeChecks)
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
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
16 changes: 13 additions & 3 deletions src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ILScanner.cs
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,12 @@ public ScannedDevirtualizationManager(NodeFactory factory, ImmutableArray<Depend

if (type != null)
{
_constructedTypes.Add(type);
_constructedMethodTables.Add(type);
TypeDesc canonForm = type.ConvertToCanonForm(CanonicalFormKind.Specific);
if (canonForm != type)
{
_canonConstructedMethodTables.Add(canonForm);
}

if (type.IsInterface)
{
Expand Down Expand Up @@ -687,7 +693,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 @@ -3280,7 +3280,7 @@ private void updateEntryPointForTailCall(ref CORINFO_CONST_LOOKUP entryPoint)
private int getExactClasses(CORINFO_CLASS_STRUCT_* baseType, int maxExactClasses, CORINFO_CLASS_STRUCT_** exactClsRet)
{
// Not implemented for R2R yet
return 0;
return -1;
}

private bool getStaticFieldContent(CORINFO_FIELD_STRUCT_* fieldHandle, byte* buffer, int bufferSize, int valueOffset, bool ignoreMovableObjects)
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 @@ -2248,14 +2248,42 @@ 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;
if (type == null)
{
return -1;
}

if (canNeverHaveInstanceOfSubclassOf(type))
{
return 0;
}

if (maxExactClasses == 0)
{
return -1;
}

// type is already sealed, return it
if (_compilation.IsEffectivelySealed(type))
{
Expand All @@ -2266,7 +2294,7 @@ private int getExactClasses(CORINFO_CLASS_STRUCT_* baseType, int maxExactClasses
TypeDesc[] implClasses = _compilation.GetImplementingClasses(type);
if (implClasses == null || implClasses.Length > maxExactClasses)
{
return 0;
return -1;
}

int index = 0;
Expand Down
8 changes: 4 additions & 4 deletions src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2787,15 +2787,15 @@ void MethodContext::recGetExactClasses(CORINFO_CLASS_HANDLE baseType, int maxExa
key.A = CastHandle(baseType);
key.B = maxExactClasses;

Assert(result >= 0);
int numResults = result < 0 ? 0 : result;

DWORDLONG* exactClassesAgnostic = new DWORDLONG[result];
for (int i = 0; i < result; i++)
DWORDLONG* exactClassesAgnostic = new DWORDLONG[numResults];
for (int i = 0; i < numResults; i++)
exactClassesAgnostic[i] = CastHandle(exactClsRet[i]);

Agnostic_GetExactClassesResult value;
value.numClasses = result;
value.classes = GetExactClasses->AddBuffer((unsigned char*)exactClassesAgnostic, (unsigned int)(result * sizeof(DWORDLONG)));
value.classes = GetExactClasses->AddBuffer((unsigned char*)exactClassesAgnostic, (unsigned int)(numResults * sizeof(DWORDLONG)));

delete[] exactClassesAgnostic;

Expand Down
4 changes: 1 addition & 3 deletions src/coreclr/vm/jitinterface.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -9663,16 +9663,14 @@ int CEEInfo::getExactClasses (
MODE_ANY;
} CONTRACTL_END;

int exactClassesCount = 0;

JIT_TO_EE_TRANSITION();

// This function is currently implemented only on NativeAOT
// but can be implemented for CoreCLR as well (e.g. for internal types)

EE_TO_JIT_TRANSITION();

return exactClassesCount;
return -1;
}

/*********************************************************************/
Expand Down
3 changes: 3 additions & 0 deletions src/tests/managed/Compilation/Compilation.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@
<GCStressIncompatible>true</GCStressIncompatible>
<!-- Test unsupported outside of windows -->
<CLRTestTargetUnsupported Condition="'$(TargetsWindows)' != 'true'">true</CLRTestTargetUnsupported>

<!-- https://github.com/dotnet/runtime/issues/99798 -->
<NativeAotIncompatible>true</NativeAotIncompatible>
</PropertyGroup>
<ItemGroup>
<Compile Include="$(MSBuildProjectName).cs" />
Expand Down
Loading