Skip to content

Commit

Permalink
Reduce duplicity in GVM analysis (#83250)
Browse files Browse the repository at this point in the history
There were two very similar pieces of logic in the generic virtual method analysis. First one was deciding whether we need to emit GVM bookkeeping about the type, the second one was deciding whether the type should participate in GVM analysis (the N*M algorithm, "for each type relevant to GVM analysis" * "GVM virtual method called").

Made this into one so that it's in one spot. The interface method resolution is not cheap so this also speeds up the compilation. We should also do this for non-generic virtuals at some point.

I did a test pass with an assert that the analysis came up with the same conclusions to ensure I didn't mess up.
  • Loading branch information
MichalStrehovsky authored Mar 14, 2023
1 parent 648d477 commit 55a0940
Show file tree
Hide file tree
Showing 6 changed files with 115 additions and 140 deletions.
5 changes: 1 addition & 4 deletions eng/testing/tests.singlefile.targets
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,10 @@
<IlcSdkPath>$(CoreCLRAotSdkDir)</IlcSdkPath>
<IlcFrameworkPath>$(NetCoreAppCurrentTestHostSharedFrameworkPath)</IlcFrameworkPath>
<IlcFrameworkNativePath>$(NetCoreAppCurrentTestHostSharedFrameworkPath)</IlcFrameworkNativePath>
<NoWarn>$(NoWarn);IL1005;IL3002</NoWarn>
<NoWarn>$(NoWarn);IL1005;IL3002;IL3003</NoWarn>
<TrimMode>partial</TrimMode>
<SuppressTrimAnalysisWarnings>true</SuppressTrimAnalysisWarnings>
<SuppressAotAnalysisWarnings>true</SuppressAotAnalysisWarnings>

<!-- Forced by ILLink targets; we should fix the SDK -->
<SelfContained>true</SelfContained>
</PropertyGroup>

<PropertyGroup Condition="'$(PublishSingleFile)' == 'true' or '$(TestNativeAot)' == 'true'">
Expand Down
14 changes: 0 additions & 14 deletions src/coreclr/tools/Common/Compiler/TypeExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -95,20 +95,6 @@ public static bool IsArrayMethod(this MethodDesc method)
arrayMethod.Kind == ArrayMethodKind.Ctor);
}

/// <summary>
/// Gets a value indicating whether this type has any generic virtual methods.
/// </summary>
public static bool HasGenericVirtualMethods(this TypeDesc type)
{
foreach (var method in type.GetAllVirtualMethods())
{
if (method.HasInstantiation)
return true;
}

return false;
}

/// <summary>
/// Wrapper helper function around the IsCanonicalDefinitionType API on the TypeSystemContext
/// </summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ protected override DependencyList ComputeNonRelocationBasedDependencies(NodeFact
dependencyList.Add(factory.NativeLayout.TemplateTypeLayout(_type), "Universal generic types always have template layout");

// Track generic virtual methods that will get added to the GVM tables
if (TypeGVMEntriesNode.TypeNeedsGVMTableEntries(_type))
if ((_virtualMethodAnalysisFlags & VirtualMethodAnalysisFlags.NeedsGvmEntries) != 0)
{
dependencyList.Add(new DependencyListEntry(factory.TypeGVMEntries(_type.GetTypeDefinition()), "Type with generic virtual methods"));

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,20 @@ public partial class EETypeNode : DehydratableObjectNode, IEETypeNode, ISymbolDe
protected bool? _mightHaveInterfaceDispatchMap;
private bool _hasConditionalDependenciesFromMetadataManager;

protected readonly VirtualMethodAnalysisFlags _virtualMethodAnalysisFlags;

[Flags]
protected enum VirtualMethodAnalysisFlags
{
None = 0,

NeedsGvmEntries = 0x0001,
InterestingForDynamicDependencies = 0x0002,

AllFlags = NeedsGvmEntries
| InterestingForDynamicDependencies,
}

public EETypeNode(NodeFactory factory, TypeDesc type)
{
if (type.IsCanonicalDefinitionType(CanonicalFormKind.Any))
Expand All @@ -82,6 +96,9 @@ public EETypeNode(NodeFactory factory, TypeDesc type)
_writableDataNode = factory.Target.SupportsRelativePointers ? new WritableDataNode(this) : null;
_hasConditionalDependenciesFromMetadataManager = factory.MetadataManager.HasConditionalDependenciesDueToEETypePresence(type);

if (EmitVirtualSlotsAndInterfaces)
_virtualMethodAnalysisFlags = AnalyzeVirtualMethods(type);

factory.TypeSystemContext.EnsureLoadableType(type);

// We don't have a representation for function pointers right now
Expand All @@ -91,6 +108,94 @@ public EETypeNode(NodeFactory factory, TypeDesc type)
static TypeDesc WithoutParameterizeTypes(TypeDesc t) => t is ParameterizedType pt ? WithoutParameterizeTypes(pt.ParameterType) : t;
}

private static VirtualMethodAnalysisFlags AnalyzeVirtualMethods(TypeDesc type)
{
var result = VirtualMethodAnalysisFlags.None;

// Interface EETypes not relevant to virtual method analysis at this time.
if (type.IsInterface)
return result;

DefType defType = type.GetClosestDefType();

foreach (MethodDesc method in defType.GetAllVirtualMethods())
{
// First, check if this type has any GVM that overrides a GVM on a parent type. If that's the case, this makes
// the current type interesting for GVM analysis (i.e. instantiate its overriding GVMs for existing GVMDependenciesNodes
// of the instantiated GVM on the parent types).
if (method.HasInstantiation)
{
result |= VirtualMethodAnalysisFlags.NeedsGvmEntries;

MethodDesc slotDecl = MetadataVirtualMethodAlgorithm.FindSlotDefiningMethodForVirtualMethod(method);
if (slotDecl != method)
result |= VirtualMethodAnalysisFlags.InterestingForDynamicDependencies;
}

// Early out if we set all the flags we could have set
if ((result & VirtualMethodAnalysisFlags.AllFlags) == VirtualMethodAnalysisFlags.AllFlags)
return result;
}

//
// Check if the type implements any interface, where the method implementations could be on
// base types.
// Example:
// interface IFace {
// void IFaceGVMethod<U>();
// }
// class BaseClass {
// public virtual void IFaceGVMethod<U>() { ... }
// }
// public class DerivedClass : BaseClass, IFace { }
//
foreach (DefType interfaceImpl in defType.RuntimeInterfaces)
{
foreach (MethodDesc method in interfaceImpl.GetAllVirtualMethods())
{
if (!method.HasInstantiation)
continue;

// We found a GVM on one of the implemented interfaces. Find if the type implements this method.
// (Note, do this comparison against the generic definition of the method, not the specific method instantiation
MethodDesc slotDecl = method.Signature.IsStatic ?
defType.ResolveInterfaceMethodToStaticVirtualMethodOnType(method)
: defType.ResolveInterfaceMethodTarget(method);
if (slotDecl != null)
{
// If the type doesn't introduce this interface method implementation (i.e. the same implementation
// already exists in the base type), do not consider this type interesting for GVM analysis just yet.
//
// We need to limit the number of types that are interesting for GVM analysis at all costs since
// these all will be looked at for every unique generic virtual method call in the program.
// Having a long list of interesting types affects the compilation throughput heavily.
if (slotDecl.OwningType == defType ||
defType.BaseType.ResolveInterfaceMethodTarget(method) != slotDecl)
{
result |= VirtualMethodAnalysisFlags.InterestingForDynamicDependencies
| VirtualMethodAnalysisFlags.NeedsGvmEntries;
}
}
else
{
// The method could be implemented by a default interface method
var resolution = defType.ResolveInterfaceMethodToDefaultImplementationOnType(method, out _);
if (resolution == DefaultInterfaceMethodResolution.DefaultImplementation)
{
result |= VirtualMethodAnalysisFlags.InterestingForDynamicDependencies
| VirtualMethodAnalysisFlags.NeedsGvmEntries;
}
}

// Early out if we set all the flags we could have set
if ((result & VirtualMethodAnalysisFlags.AllFlags) == VirtualMethodAnalysisFlags.AllFlags)
return result;
}
}

return result;
}

protected bool MightHaveInterfaceDispatchMap(NodeFactory factory)
{
if (!_mightHaveInterfaceDispatchMap.HasValue)
Expand Down Expand Up @@ -135,78 +240,7 @@ public static int GetMinimumObjectSize(TypeSystemContext typeSystemContext)
protected virtual bool EmitVirtualSlotsAndInterfaces => false;

public override bool InterestingForDynamicDependencyAnalysis
{
get
{
if (!EmitVirtualSlotsAndInterfaces)
return false;

if (_type.IsInterface)
return false;

if (_type.IsDefType)
{
// First, check if this type has any GVM that overrides a GVM on a parent type. If that's the case, this makes
// the current type interesting for GVM analysis (i.e. instantiate its overriding GVMs for existing GVMDependenciesNodes
// of the instantiated GVM on the parent types).
foreach (var method in _type.GetAllVirtualMethods())
{
Debug.Assert(method.IsVirtual);

if (method.HasInstantiation)
{
MethodDesc slotDecl = MetadataVirtualMethodAlgorithm.FindSlotDefiningMethodForVirtualMethod(method);
if (slotDecl != method)
return true;
}
}

// Second, check if this type has any GVMs that implement any GVM on any of the implemented interfaces. This would
// make the current type interesting for dynamic dependency analysis to that we can instantiate its GVMs.
foreach (DefType interfaceImpl in _type.RuntimeInterfaces)
{
foreach (var method in interfaceImpl.GetAllVirtualMethods())
{
Debug.Assert(method.IsVirtual);

if (method.HasInstantiation)
{
// We found a GVM on one of the implemented interfaces. Find if the type implements this method.
// (Note, do this comparison against the generic definition of the method, not the specific method instantiation
MethodDesc genericDefinition = method.GetMethodDefinition();
MethodDesc slotDecl = genericDefinition.Signature.IsStatic ?
_type.ResolveInterfaceMethodToStaticVirtualMethodOnType(genericDefinition)
: _type.ResolveInterfaceMethodTarget(genericDefinition);
if (slotDecl != null)
{
// If the type doesn't introduce this interface method implementation (i.e. the same implementation
// already exists in the base type), do not consider this type interesting for GVM analysis just yet.
//
// We need to limit the number of types that are interesting for GVM analysis at all costs since
// these all will be looked at for every unique generic virtual method call in the program.
// Having a long list of interesting types affects the compilation throughput heavily.
if (slotDecl.OwningType == _type ||
_type.BaseType.ResolveInterfaceMethodTarget(genericDefinition) != slotDecl)
{
return true;
}
}
else
{
// The method could be implemented by a default interface method
var resolution = _type.ResolveInterfaceMethodToDefaultImplementationOnType(genericDefinition, out slotDecl);
if (resolution == DefaultInterfaceMethodResolution.DefaultImplementation)
{
return true;
}
}
}
}
}
}
return false;
}
}
=> (_virtualMethodAnalysisFlags & VirtualMethodAnalysisFlags.InterestingForDynamicDependencies) != 0;

internal bool HasOptionalFields
{
Expand Down Expand Up @@ -555,7 +589,7 @@ protected override DependencyList ComputeNonRelocationBasedDependencies(NodeFact
dependencies.Add(factory.VTable(intface), "Interface vtable slice");

// Generated type contains generic virtual methods that will get added to the GVM tables
if (TypeGVMEntriesNode.TypeNeedsGVMTableEntries(_type))
if ((_virtualMethodAnalysisFlags & VirtualMethodAnalysisFlags.NeedsGvmEntries) != 0)
{
dependencies.Add(new DependencyListEntry(factory.TypeGVMEntries(_type.GetTypeDefinition()), "Type with generic virtual methods"));

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@ public InterfaceGVMEntryInfo(MethodDesc callingMethod, MethodDesc implementation
public TypeGVMEntriesNode(TypeDesc associatedType)
{
Debug.Assert(associatedType.IsTypeDefinition);
Debug.Assert(TypeNeedsGVMTableEntries(associatedType));
_associatedType = associatedType;
}

Expand All @@ -71,56 +70,11 @@ public override IEnumerable<DependencyListEntry> GetStaticDependencies(NodeFacto

foreach (var entry in ScanForInterfaceGenericVirtualMethodEntries())
InterfaceGenericVirtualMethodTableNode.GetGenericVirtualMethodImplementationDependencies(ref _staticDependencies, context, entry.CallingMethod, entry.ImplementationType, entry.ImplementationMethod);
}

return _staticDependencies;
}

public static bool TypeNeedsGVMTableEntries(TypeDesc type)
{
// Only non-interface deftypes can have entries for their GVMs in the GVM hashtables.
// Interface GVM entries are computed for types that implemenent the interface (not for the interface on its own)
if (!type.IsDefType || type.IsInterface)
return false;

// Type declares GVMs
if (type.HasGenericVirtualMethods())
return true;

//
// Check if the type implements any interface with GVM methods, where the method implementations could be on
// base types.
// Example:
// interface IFace {
// void IFaceGVMethod<U>();
// }
// class BaseClass {
// public virtual void IFaceGVMethod<U>() { ... }
// }
// public class DerivedClass : BaseClass, IFace { }
//
foreach (var iface in type.RuntimeInterfaces)
{
foreach (var method in iface.GetVirtualMethods())
{
if (!method.HasInstantiation)
continue;

MethodDesc slotDecl = method.Signature.IsStatic ?
type.ResolveInterfaceMethodToStaticVirtualMethodOnType(method) : type.ResolveInterfaceMethodTarget(method);
if (slotDecl == null)
{
var resolution = type.ResolveInterfaceMethodToDefaultImplementationOnType(method, out slotDecl);
if (resolution == DefaultInterfaceMethodResolution.None)
slotDecl = null;
}

if (slotDecl != null)
return true;
}
Debug.Assert(_staticDependencies.Count > 0);
}

return false;
return _staticDependencies;
}

public IEnumerable<TypeGVMEntryInfo> ScanForGenericVirtualMethodEntries()
Expand Down
4 changes: 4 additions & 0 deletions src/libraries/tests.proj
Original file line number Diff line number Diff line change
Expand Up @@ -417,6 +417,10 @@
</ItemGroup>

<ItemGroup Condition="'$(TestNativeAot)' == 'true' and '$(RunDisabledNativeAotTests)' != 'true'">
<!-- https://github.com/dotnet/runtime/issues/83167 -->
<ProjectExclusions Include="$(MSBuildThisFileDirectory)System.Numerics.Vectors\tests\System.Numerics.Vectors.Tests.csproj"
Condition="'$(TargetArchitecture)' == 'arm64'" />

<!-- https://github.com/dotnet/runtime/issues/73431 -->
<ProjectExclusions Include="$(MSBuildThisFileDirectory)System.Text.Json\tests\System.Text.Json.SourceGeneration.Tests\System.Text.Json.SourceGeneration.Roslyn3.11.Tests.csproj" />
<ProjectExclusions Include="$(MSBuildThisFileDirectory)System.Text.Json\tests\System.Text.Json.SourceGeneration.Tests\System.Text.Json.SourceGeneration.Roslyn4.4.Tests.csproj" />
Expand Down

0 comments on commit 55a0940

Please sign in to comment.