From bbeb1f3711d1a62e6535e274087c270c3747eac5 Mon Sep 17 00:00:00 2001 From: Tomas Date: Fri, 11 Jun 2021 14:33:45 +0200 Subject: [PATCH 01/11] WIP: Crossgen2 support for static virtual method resolution This is my initial attempt at porting the CoreCLR runtime code for SVM resolution to Crossgen2. The TypeHierarchyTest now fails at 66-th scenario. The biggest problems I'm hitting are around making sure that we drop the constraint from the actual entrypoint import cells, otherwise we end up with constructs like BaseScenario61.Method() @ DerivedScenario61 and that crashes the runtime. I guess that my treatment of generics is most likely still incorrect. Thanks Tomas --- .../ReadyToRun/SignatureBuilder.cs | 5 +- .../JitInterface/CorInfoImpl.ReadyToRun.cs | 143 ++++++++++++++++-- 2 files changed, 133 insertions(+), 15 deletions(-) diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/SignatureBuilder.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/SignatureBuilder.cs index 4f0fc3ddb21c4f..42f56000991c73 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/SignatureBuilder.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/SignatureBuilder.cs @@ -400,6 +400,7 @@ public void EmitMethodSignature( bool isInstantiatingStub) { uint flags = 0; + bool useConstraint = (method.ConstrainedType != null && method.ConstrainedType != method.OwningType); if (method.Unboxing) { flags |= (uint)ReadyToRunMethodSigFlags.READYTORUN_METHOD_SIG_UnboxingStub; @@ -408,7 +409,7 @@ public void EmitMethodSignature( { flags |= (uint)ReadyToRunMethodSigFlags.READYTORUN_METHOD_SIG_InstantiatingStub; } - if (method.ConstrainedType != null) + if (useConstraint) { flags |= (uint)ReadyToRunMethodSigFlags.READYTORUN_METHOD_SIG_Constrained; } @@ -419,7 +420,7 @@ public void EmitMethodSignature( EmitMethodSpecificationSignature(method, flags, enforceDefEncoding, enforceOwningType, context); - if (method.ConstrainedType != null) + if (useConstraint) { EmitTypeSignature(method.ConstrainedType, context); } diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs index e3da2dc734fef5..a7c29354a07fdb 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs @@ -1459,11 +1459,7 @@ private void ceeInfoGetCallInfo( exactType = type; - constrainedType = null; - if ((flags & CORINFO_CALLINFO_FLAGS.CORINFO_CALLINFO_CALLVIRT) != 0 && pConstrainedResolvedToken != null) - { - constrainedType = HandleToObject(pConstrainedResolvedToken->hClass); - } + constrainedType = (pConstrainedResolvedToken != null ? HandleToObject(pConstrainedResolvedToken->hClass) : null); bool resolvedConstraint = false; bool forceUseRuntimeLookup = false; @@ -1490,7 +1486,16 @@ private void ceeInfoGetCallInfo( originalMethod = methodOnUnderlyingType; } - MethodDesc directMethod = constrainedType.TryResolveConstraintMethodApprox(exactType, originalMethod, out forceUseRuntimeLookup); + MethodDesc directMethod; + if (originalMethod.Signature.IsStatic) + { + directMethod = ResolveVirtualStaticMethod(constrainedType, exactType, originalMethod, allowVariantMatches: true); + forceUseRuntimeLookup = (directMethod == null); + } + else + { + directMethod = constrainedType.TryResolveConstraintMethodApprox(exactType, originalMethod, out forceUseRuntimeLookup); + } if (directMethod != null) { // Either @@ -1512,6 +1517,7 @@ private void ceeInfoGetCallInfo( pResult->thisTransform = CORINFO_THIS_TRANSFORM.CORINFO_NO_THIS_TRANSFORM; exactType = constrainedType; + constrainedType = directMethod.OwningType; } else if (constrainedType.IsValueType) { @@ -1841,13 +1847,6 @@ private void VerifyMethodSignatureIsStable(MethodSignature methodSig) private void getCallInfo(ref CORINFO_RESOLVED_TOKEN pResolvedToken, CORINFO_RESOLVED_TOKEN* pConstrainedResolvedToken, CORINFO_METHOD_STRUCT_* callerHandle, CORINFO_CALLINFO_FLAGS flags, CORINFO_CALL_INFO* pResult) { - if ((flags & CORINFO_CALLINFO_FLAGS.CORINFO_CALLINFO_CALLVIRT) == 0 && pConstrainedResolvedToken != null) - { - // Defer constrained call / ldftn instructions used for static virtual methods - // to runtime resolution. - throw new RequiresRuntimeJitException("SVM"); - } - MethodDesc methodToCall; MethodDesc targetMethod; TypeDesc constrainedType; @@ -2627,5 +2626,123 @@ private void reportInliningDecision(CORINFO_METHOD_STRUCT_* inlinerHnd, CORINFO_ _inlinedMethods.Add(inlinee); } } + + /// + /// Try to resolve a given virtual static interface method on a given constrained type and its base types. + /// + /// Type to attempt virtual static method resolution on + /// Interface declaring the method + /// Interface method to resolve + /// True when variant matches are allowed + /// MethodDesc of the resolved virtual static method, null when not found (runtime lookup must be used) + private static MethodDesc ResolveVirtualStaticMethod(TypeDesc constrainedType, TypeDesc interfaceType, MethodDesc interfaceMethod, bool allowVariantMatches) + { + if (interfaceMethod.IsSharedByGenericInstantiations || interfaceType.IsCanonicalSubtype(CanonicalFormKind.Any)) + { + return null; + } + + // Check that there is no implementation of the interface on this type which is the canonical interface for a shared generic. If so, that indicates that + // we cannot exactly compute a target method result, as even if there is an exact match in the type hierarchy + // it isn't guaranteed that we will always find the right result, as we may find a match on a base type when we should find the match + // on a more derived type. + TypeDesc interfaceTypeCanonical = interfaceType.ConvertToCanonForm(CanonicalFormKind.Specific); + if (interfaceType != interfaceTypeCanonical) + { + foreach (DefType runtimeInterface in constrainedType.RuntimeInterfaces) + { + if (interfaceTypeCanonical == runtimeInterface) + { + return null; + } + } + } + + // Search for match on a per-level in the type hierarchy + for (TypeDesc typeToCheck = constrainedType; typeToCheck != null; typeToCheck = typeToCheck.BaseType) + { + MethodDesc resolvedMethodOnType = TryResolveVirtualStaticMethodOnThisType(typeToCheck, interfaceType, interfaceMethod); + if (resolvedMethodOnType != null) + { + return resolvedMethodOnType; + } + + if (interfaceType.HasVariance /* TODO || interfaceType.HasTypeEquivalence */) + { + // Variant interface dispatch + foreach (DefType runtimeInterfaceType in typeToCheck.RuntimeInterfaces) + { + if (runtimeInterfaceType == interfaceType) + { + // This is the variant interface check logic, skip this + continue; + } + + if (!runtimeInterfaceType.HasSameTypeDefinition(interfaceType)) + { + // Variance matches require a typedef match + // Equivalence isn't sufficient, and is uninteresting as equivalent interfaces cannot have static virtuals. + continue; + } + + bool equivalentOrVariantCompatible; + + if (allowVariantMatches) + { + equivalentOrVariantCompatible = runtimeInterfaceType.CanCastTo(interfaceType); + } + else + { + // When performing override checking to ensure that a concrete type is valid, require the implementation + // actually implement the exact or equivalent interface. + equivalentOrVariantCompatible = false; // TODO: runtimeInterfaceType.IsEquivalentTo(interfaceType); + } + + if (equivalentOrVariantCompatible) + { + // Variant or equivalent matching interface found + // Attempt to resolve on variance matched interface + resolvedMethodOnType = TryResolveVirtualStaticMethodOnThisType(typeToCheck, runtimeInterfaceType, interfaceMethod); + if (resolvedMethodOnType != null) + { + return resolvedMethodOnType; + } + } + } + } + } + return null; + } + + /// + /// Try to resolve a given virtual static interface method on a given constrained type and return the resolved method or null when not found. + /// + /// Type to attempt method resolution on + /// Interface declaring the method + /// Method to resolve + /// MethodDesc of the resolved method or null when not found (runtime lookup must be used) + private static MethodDesc TryResolveVirtualStaticMethodOnThisType(TypeDesc constrainedType, TypeDesc interfaceType, MethodDesc interfaceMethod) + { + if (constrainedType is MetadataType mdType) + { + foreach (MethodImplRecord methodImpl in mdType.FindMethodsImplWithMatchingDeclName(interfaceMethod.Name) ?? Array.Empty()) + { + if (methodImpl.Decl.OwningType == interfaceType && + methodImpl.Decl == interfaceMethod) + { + MethodDesc resolvedMethodImpl = methodImpl.Body; + if (interfaceMethod.HasInstantiation || methodImpl.Body.HasInstantiation || constrainedType.HasInstantiation) + { + resolvedMethodImpl = resolvedMethodImpl.InstantiateSignature(constrainedType.Instantiation, interfaceMethod.Instantiation); + } + if (resolvedMethodImpl != null) + { + return resolvedMethodImpl; + } + } + } + } + return null; + } } } From 6191fa3e2390f1902a35fff49d42f1c58af8fb74 Mon Sep 17 00:00:00 2001 From: Tomas Date: Sun, 20 Jun 2021 21:57:43 +0200 Subject: [PATCH 02/11] Address initial David's PR feedback I'm adding this as a separate commit as it's basically just a projection of David's PR suggestions without additional algorithmic changes. I'll follow up with additional fixes for the remaining test failures. Thanks Tomas --- .../Compiler/DevirtualizationManager.cs | 43 ++++++ .../Common/MetadataVirtualMethodAlgorithm.cs | 97 +++++++++++++ .../TypeSystem/Common/TypeSystemHelpers.cs | 5 + .../Common/VirtualMethodAlgorithm.cs | 2 + .../ReadyToRun/DevirtualizationManager.cs | 43 ++++++ .../ReadyToRun/SignatureBuilder.cs | 5 +- .../Compiler/ReadyToRunCodegenCompilation.cs | 5 + .../JitInterface/CorInfoImpl.ReadyToRun.cs | 128 ++---------------- 8 files changed, 205 insertions(+), 123 deletions(-) diff --git a/src/coreclr/tools/Common/Compiler/DevirtualizationManager.cs b/src/coreclr/tools/Common/Compiler/DevirtualizationManager.cs index aba41341a27095..e9e530bee16b7b 100644 --- a/src/coreclr/tools/Common/Compiler/DevirtualizationManager.cs +++ b/src/coreclr/tools/Common/Compiler/DevirtualizationManager.cs @@ -201,5 +201,48 @@ protected virtual MethodDesc ResolveVirtualMethod(MethodDesc declMethod, DefType return impl; } + + /// + /// Check whether compile-time resolution of static virtual method is possible. This returns false for shared + /// generics where the resolution needs to be deferred to the runtime working with exact instantiation types. + /// + /// Type constraint in the static virtual method + /// Static virtual interface method to resolve + /// Exact interface type for the static virtual method + public bool AllowCompileTimeStaticVirtualMethodResolution(TypeDesc currentType, MethodDesc interfaceMethod, TypeDesc interfaceType) + { + Debug.Assert(interfaceMethod.IsVirtual); + Debug.Assert(interfaceMethod.Signature.IsStatic); + Debug.Assert(interfaceType.IsInterface); + Debug.Assert(!currentType.IsInterface); + + return AllowCompileTimeStaticVirtualMethodResolution(currentType.GetClosestDefType(), interfaceMethod, interfaceType); + } + + protected virtual bool AllowCompileTimeStaticVirtualMethodResolution(DefType currentType, MethodDesc interfaceMethod, TypeDesc interfaceType) + { + if (interfaceMethod.IsSharedByGenericInstantiations || interfaceType.IsCanonicalSubtype(CanonicalFormKind.Any)) + { + return false; + } + + // Check that there is no implementation of the interface on this type which is the canonical interface for a shared generic. If so, that indicates that + // we cannot exactly compute a target method result, as even if there is an exact match in the type hierarchy + // it isn't guaranteed that we will always find the right result, as we may find a match on a base type when we should find the match + // on a more derived type. + TypeDesc interfaceTypeCanonical = interfaceType.ConvertToCanonForm(CanonicalFormKind.Specific); + if (interfaceType != interfaceTypeCanonical) + { + foreach (DefType runtimeInterface in currentType.RuntimeInterfaces) + { + if (interfaceTypeCanonical == runtimeInterface) + { + return false; + } + } + } + + return true; + } } } diff --git a/src/coreclr/tools/Common/TypeSystem/Common/MetadataVirtualMethodAlgorithm.cs b/src/coreclr/tools/Common/TypeSystem/Common/MetadataVirtualMethodAlgorithm.cs index 8b878afcc0c061..de1f308f2ce8b9 100644 --- a/src/coreclr/tools/Common/TypeSystem/Common/MetadataVirtualMethodAlgorithm.cs +++ b/src/coreclr/tools/Common/TypeSystem/Common/MetadataVirtualMethodAlgorithm.cs @@ -826,5 +826,102 @@ public static IEnumerable EnumAllVirtualSlots(MetadataType type) } while (type != null); } } + + /// + /// Try to resolve a given virtual static interface method on a given constrained type and its base types. + /// + /// Type to attempt virtual static method resolution on + /// Interface declaring the method + /// Interface method to resolve + /// True when variant matches are allowed + /// MethodDesc of the resolved virtual static method, null when not found (runtime lookup must be used) + public override MethodDesc ResolveInterfaceMethodToStaticVirtualMethodOnType(MethodDesc interfaceMethod, TypeDesc interfaceType, TypeDesc currentType, bool allowVariantMatches) + { + // Search for match on a per-level in the type hierarchy + for (TypeDesc typeToCheck = currentType; typeToCheck != null; typeToCheck = typeToCheck.BaseType) + { + MethodDesc resolvedMethodOnType = TryResolveVirtualStaticMethodOnThisType(typeToCheck, interfaceType, interfaceMethod); + if (resolvedMethodOnType != null) + { + return resolvedMethodOnType; + } + + if (interfaceType.HasVariance /* TODO || interfaceType.HasTypeEquivalence */) + { + // Variant interface dispatch + foreach (DefType runtimeInterfaceType in typeToCheck.RuntimeInterfaces) + { + if (runtimeInterfaceType == interfaceType) + { + // This is the variant interface check logic, skip this + continue; + } + + if (!runtimeInterfaceType.HasSameTypeDefinition(interfaceType)) + { + // Variance matches require a typedef match + // Equivalence isn't sufficient, and is uninteresting as equivalent interfaces cannot have static virtuals. + continue; + } + + bool equivalentOrVariantCompatible; + + if (allowVariantMatches) + { + equivalentOrVariantCompatible = runtimeInterfaceType.CanCastTo(interfaceType); + } + else + { + // When performing override checking to ensure that a concrete type is valid, require the implementation + // actually implement the exact or equivalent interface. + equivalentOrVariantCompatible = false; // TODO: runtimeInterfaceType.IsEquivalentTo(interfaceType); + } + + if (equivalentOrVariantCompatible) + { + // Variant or equivalent matching interface found + // Attempt to resolve on variance matched interface + resolvedMethodOnType = TryResolveVirtualStaticMethodOnThisType(typeToCheck, runtimeInterfaceType, interfaceMethod); + if (resolvedMethodOnType != null) + { + return resolvedMethodOnType; + } + } + } + } + } + return null; + } + + /// + /// Try to resolve a given virtual static interface method on a given constrained type and return the resolved method or null when not found. + /// + /// Type to attempt method resolution on + /// Interface declaring the method + /// Method to resolve + /// MethodDesc of the resolved method or null when not found (runtime lookup must be used) + private static MethodDesc TryResolveVirtualStaticMethodOnThisType(TypeDesc constrainedType, TypeDesc interfaceType, MethodDesc interfaceMethod) + { + if (constrainedType is MetadataType mdType) + { + foreach (MethodImplRecord methodImpl in mdType.FindMethodsImplWithMatchingDeclName(interfaceMethod.Name) ?? Array.Empty()) + { + if (methodImpl.Decl.OwningType == interfaceType && + methodImpl.Decl == interfaceMethod) + { + MethodDesc resolvedMethodImpl = methodImpl.Body; + if (interfaceMethod.HasInstantiation || methodImpl.Body.HasInstantiation || constrainedType.HasInstantiation) + { + resolvedMethodImpl = resolvedMethodImpl.InstantiateSignature(constrainedType.Instantiation, interfaceMethod.Instantiation); + } + if (resolvedMethodImpl != null) + { + return resolvedMethodImpl; + } + } + } + } + return null; + } } } diff --git a/src/coreclr/tools/Common/TypeSystem/Common/TypeSystemHelpers.cs b/src/coreclr/tools/Common/TypeSystem/Common/TypeSystemHelpers.cs index 3732586a45e75a..2a734bdec6c24f 100644 --- a/src/coreclr/tools/Common/TypeSystem/Common/TypeSystemHelpers.cs +++ b/src/coreclr/tools/Common/TypeSystem/Common/TypeSystemHelpers.cs @@ -293,6 +293,11 @@ public static DefaultInterfaceMethodResolution ResolveInterfaceMethodToDefaultIm return type.Context.GetVirtualMethodAlgorithmForType(type).ResolveInterfaceMethodToDefaultImplementationOnType(interfaceMethod, type, out implMethod); } + public static MethodDesc ResolveInterfaceMethodToStaticVirtualMethodOnType(this TypeDesc type, MethodDesc interfaceMethod, TypeDesc interfaceType, bool allowVariantMatches) + { + return type.Context.GetVirtualMethodAlgorithmForType(type).ResolveInterfaceMethodToStaticVirtualMethodOnType(interfaceMethod, interfaceType, type, allowVariantMatches); + } + /// /// Resolves a virtual method call. /// diff --git a/src/coreclr/tools/Common/TypeSystem/Common/VirtualMethodAlgorithm.cs b/src/coreclr/tools/Common/TypeSystem/Common/VirtualMethodAlgorithm.cs index e13e61d3862da6..85a1d78867bc88 100644 --- a/src/coreclr/tools/Common/TypeSystem/Common/VirtualMethodAlgorithm.cs +++ b/src/coreclr/tools/Common/TypeSystem/Common/VirtualMethodAlgorithm.cs @@ -24,6 +24,8 @@ public abstract class VirtualMethodAlgorithm public abstract MethodDesc ResolveVariantInterfaceMethodToVirtualMethodOnType(MethodDesc interfaceMethod, TypeDesc currentType); + public abstract MethodDesc ResolveInterfaceMethodToStaticVirtualMethodOnType(MethodDesc interfaceMethod, TypeDesc exactInterfaceType, TypeDesc currentType, bool allowVariantMatches); + public abstract DefaultInterfaceMethodResolution ResolveInterfaceMethodToDefaultImplementationOnType(MethodDesc interfaceMethod, TypeDesc currentType, out MethodDesc impl); /// diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/DevirtualizationManager.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/DevirtualizationManager.cs index ab85bb84936f71..bc6dc728bd13aa 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/DevirtualizationManager.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/DevirtualizationManager.cs @@ -211,5 +211,48 @@ static TypeDesc FindVersionBubbleEdge(CompilationModuleGroup compilationModuleGr return type; } } + + private enum SVMResolutionEligibility : byte + { + Allow, + Forbid, + NotApplicable, + } + + protected override bool AllowCompileTimeStaticVirtualMethodResolution(DefType currentType, MethodDesc interfaceMethod, TypeDesc interfaceType) + { + if (!base.AllowCompileTimeStaticVirtualMethodResolution(currentType, interfaceMethod, interfaceType)) + { + return false; + } + + return AllowCompileTimeStaticVirtualMethodResolutionRecursive(currentType, interfaceMethod, interfaceType) != SVMResolutionEligibility.Forbid; + } + + private SVMResolutionEligibility AllowCompileTimeStaticVirtualMethodResolutionRecursive(DefType currentType, MethodDesc interfaceMethod, TypeDesc interfaceType) + { + if (currentType.HasBaseType) + { + SVMResolutionEligibility allowParent = AllowCompileTimeStaticVirtualMethodResolutionRecursive(currentType.BaseType, interfaceMethod, interfaceType); + if (allowParent == SVMResolutionEligibility.Forbid) + { + return SVMResolutionEligibility.Forbid; + } + if (allowParent == SVMResolutionEligibility.Allow) + { + return _compilationModuleGroup.VersionsWithType(currentType.BaseType) ? SVMResolutionEligibility.Allow : SVMResolutionEligibility.Forbid; + } + } + + foreach (DefType runtimeInterface in currentType.RuntimeInterfaces) + { + if (runtimeInterface == interfaceType) + { + return _compilationModuleGroup.VersionsWithType(interfaceType) ? SVMResolutionEligibility.Allow : SVMResolutionEligibility.Forbid; + } + } + + return SVMResolutionEligibility.NotApplicable; + } } } diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/SignatureBuilder.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/SignatureBuilder.cs index 42f56000991c73..4f0fc3ddb21c4f 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/SignatureBuilder.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/SignatureBuilder.cs @@ -400,7 +400,6 @@ public void EmitMethodSignature( bool isInstantiatingStub) { uint flags = 0; - bool useConstraint = (method.ConstrainedType != null && method.ConstrainedType != method.OwningType); if (method.Unboxing) { flags |= (uint)ReadyToRunMethodSigFlags.READYTORUN_METHOD_SIG_UnboxingStub; @@ -409,7 +408,7 @@ public void EmitMethodSignature( { flags |= (uint)ReadyToRunMethodSigFlags.READYTORUN_METHOD_SIG_InstantiatingStub; } - if (useConstraint) + if (method.ConstrainedType != null) { flags |= (uint)ReadyToRunMethodSigFlags.READYTORUN_METHOD_SIG_Constrained; } @@ -420,7 +419,7 @@ public void EmitMethodSignature( EmitMethodSpecificationSignature(method, flags, enforceDefEncoding, enforceOwningType, context); - if (useConstraint) + if (method.ConstrainedType != null) { EmitTypeSignature(method.ConstrainedType, context); } diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilation.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilation.cs index ac7058bf90daec..db8c53b709f41c 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilation.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilation.cs @@ -128,6 +128,11 @@ public MethodDesc ResolveVirtualMethod(MethodDesc declMethod, TypeDesc implType, return _devirtualizationManager.ResolveVirtualMethod(declMethod, implType, out devirtualizationDetail); } + public bool AllowCompileTimeStaticVirtualMethodResolution(TypeDesc implType, MethodDesc interfaceMethod, TypeDesc interfaceType) + { + return _devirtualizationManager.AllowCompileTimeStaticVirtualMethodResolution(implType, interfaceMethod, interfaceType); + } + public bool IsModuleInstrumented(ModuleDesc module) { return _modulesBeingInstrumented.Contains(module); diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs index a7c29354a07fdb..904fd9dcd845c0 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs @@ -1489,7 +1489,14 @@ private void ceeInfoGetCallInfo( MethodDesc directMethod; if (originalMethod.Signature.IsStatic) { - directMethod = ResolveVirtualStaticMethod(constrainedType, exactType, originalMethod, allowVariantMatches: true); + if (_compilation.AllowCompileTimeStaticVirtualMethodResolution(constrainedType, originalMethod, exactType)) + { + directMethod = constrainedType.ResolveInterfaceMethodToStaticVirtualMethodOnType(originalMethod, exactType, allowVariantMatches: true); + } + else + { + directMethod = null; + } forceUseRuntimeLookup = (directMethod == null); } else @@ -1517,7 +1524,6 @@ private void ceeInfoGetCallInfo( pResult->thisTransform = CORINFO_THIS_TRANSFORM.CORINFO_NO_THIS_TRANSFORM; exactType = constrainedType; - constrainedType = directMethod.OwningType; } else if (constrainedType.IsValueType) { @@ -2626,123 +2632,5 @@ private void reportInliningDecision(CORINFO_METHOD_STRUCT_* inlinerHnd, CORINFO_ _inlinedMethods.Add(inlinee); } } - - /// - /// Try to resolve a given virtual static interface method on a given constrained type and its base types. - /// - /// Type to attempt virtual static method resolution on - /// Interface declaring the method - /// Interface method to resolve - /// True when variant matches are allowed - /// MethodDesc of the resolved virtual static method, null when not found (runtime lookup must be used) - private static MethodDesc ResolveVirtualStaticMethod(TypeDesc constrainedType, TypeDesc interfaceType, MethodDesc interfaceMethod, bool allowVariantMatches) - { - if (interfaceMethod.IsSharedByGenericInstantiations || interfaceType.IsCanonicalSubtype(CanonicalFormKind.Any)) - { - return null; - } - - // Check that there is no implementation of the interface on this type which is the canonical interface for a shared generic. If so, that indicates that - // we cannot exactly compute a target method result, as even if there is an exact match in the type hierarchy - // it isn't guaranteed that we will always find the right result, as we may find a match on a base type when we should find the match - // on a more derived type. - TypeDesc interfaceTypeCanonical = interfaceType.ConvertToCanonForm(CanonicalFormKind.Specific); - if (interfaceType != interfaceTypeCanonical) - { - foreach (DefType runtimeInterface in constrainedType.RuntimeInterfaces) - { - if (interfaceTypeCanonical == runtimeInterface) - { - return null; - } - } - } - - // Search for match on a per-level in the type hierarchy - for (TypeDesc typeToCheck = constrainedType; typeToCheck != null; typeToCheck = typeToCheck.BaseType) - { - MethodDesc resolvedMethodOnType = TryResolveVirtualStaticMethodOnThisType(typeToCheck, interfaceType, interfaceMethod); - if (resolvedMethodOnType != null) - { - return resolvedMethodOnType; - } - - if (interfaceType.HasVariance /* TODO || interfaceType.HasTypeEquivalence */) - { - // Variant interface dispatch - foreach (DefType runtimeInterfaceType in typeToCheck.RuntimeInterfaces) - { - if (runtimeInterfaceType == interfaceType) - { - // This is the variant interface check logic, skip this - continue; - } - - if (!runtimeInterfaceType.HasSameTypeDefinition(interfaceType)) - { - // Variance matches require a typedef match - // Equivalence isn't sufficient, and is uninteresting as equivalent interfaces cannot have static virtuals. - continue; - } - - bool equivalentOrVariantCompatible; - - if (allowVariantMatches) - { - equivalentOrVariantCompatible = runtimeInterfaceType.CanCastTo(interfaceType); - } - else - { - // When performing override checking to ensure that a concrete type is valid, require the implementation - // actually implement the exact or equivalent interface. - equivalentOrVariantCompatible = false; // TODO: runtimeInterfaceType.IsEquivalentTo(interfaceType); - } - - if (equivalentOrVariantCompatible) - { - // Variant or equivalent matching interface found - // Attempt to resolve on variance matched interface - resolvedMethodOnType = TryResolveVirtualStaticMethodOnThisType(typeToCheck, runtimeInterfaceType, interfaceMethod); - if (resolvedMethodOnType != null) - { - return resolvedMethodOnType; - } - } - } - } - } - return null; - } - - /// - /// Try to resolve a given virtual static interface method on a given constrained type and return the resolved method or null when not found. - /// - /// Type to attempt method resolution on - /// Interface declaring the method - /// Method to resolve - /// MethodDesc of the resolved method or null when not found (runtime lookup must be used) - private static MethodDesc TryResolveVirtualStaticMethodOnThisType(TypeDesc constrainedType, TypeDesc interfaceType, MethodDesc interfaceMethod) - { - if (constrainedType is MetadataType mdType) - { - foreach (MethodImplRecord methodImpl in mdType.FindMethodsImplWithMatchingDeclName(interfaceMethod.Name) ?? Array.Empty()) - { - if (methodImpl.Decl.OwningType == interfaceType && - methodImpl.Decl == interfaceMethod) - { - MethodDesc resolvedMethodImpl = methodImpl.Body; - if (interfaceMethod.HasInstantiation || methodImpl.Body.HasInstantiation || constrainedType.HasInstantiation) - { - resolvedMethodImpl = resolvedMethodImpl.InstantiateSignature(constrainedType.Instantiation, interfaceMethod.Instantiation); - } - if (resolvedMethodImpl != null) - { - return resolvedMethodImpl; - } - } - } - } - return null; - } } } From d5cd7a49a446176c91aa69e22b77ff3e995d7db5 Mon Sep 17 00:00:00 2001 From: Tomas Date: Sun, 20 Jun 2021 22:58:21 +0200 Subject: [PATCH 03/11] Address remaining David's PR feedback --- .../Common/MetadataVirtualMethodAlgorithm.cs | 4 ++ .../JitInterface/CorInfoImpl.ReadyToRun.cs | 39 ++++++++++++++++--- 2 files changed, 38 insertions(+), 5 deletions(-) diff --git a/src/coreclr/tools/Common/TypeSystem/Common/MetadataVirtualMethodAlgorithm.cs b/src/coreclr/tools/Common/TypeSystem/Common/MetadataVirtualMethodAlgorithm.cs index de1f308f2ce8b9..c7d0d4f82011ec 100644 --- a/src/coreclr/tools/Common/TypeSystem/Common/MetadataVirtualMethodAlgorithm.cs +++ b/src/coreclr/tools/Common/TypeSystem/Common/MetadataVirtualMethodAlgorithm.cs @@ -910,6 +910,10 @@ private static MethodDesc TryResolveVirtualStaticMethodOnThisType(TypeDesc const methodImpl.Decl == interfaceMethod) { MethodDesc resolvedMethodImpl = methodImpl.Body; + if (resolvedMethodImpl.OwningType != mdType) + { + ThrowHelper.ThrowMissingMethodException(constrainedType, resolvedMethodImpl.Name, resolvedMethodImpl.Signature); + } if (interfaceMethod.HasInstantiation || methodImpl.Body.HasInstantiation || constrainedType.HasInstantiation) { resolvedMethodImpl = resolvedMethodImpl.InstantiateSignature(constrainedType.Instantiation, interfaceMethod.Instantiation); diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs index 904fd9dcd845c0..ea4d7198365993 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs @@ -1448,11 +1448,12 @@ private void ceeInfoGetCallInfo( } callerModule = ((EcmaMethod)callerMethod.GetTypicalMethodDefinition()).Module; + bool isCallVirt = (flags & CORINFO_CALLINFO_FLAGS.CORINFO_CALLINFO_CALLVIRT) != 0; // Spec says that a callvirt lookup ignores static methods. Since static methods // can't have the exact same signature as instance methods, a lookup that found // a static method would have never found an instance method. - if (originalMethod.Signature.IsStatic && (flags & CORINFO_CALLINFO_FLAGS.CORINFO_CALLINFO_CALLVIRT) != 0) + if (originalMethod.Signature.IsStatic && isCallVirt) { ThrowHelper.ThrowInvalidProgramException(ExceptionStringID.InvalidProgramCallVirtStatic, originalMethod); } @@ -1584,7 +1585,7 @@ private void ceeInfoGetCallInfo( // Static methods are always direct calls directCall = true; } - else if ((flags & CORINFO_CALLINFO_FLAGS.CORINFO_CALLINFO_CALLVIRT) == 0 || resolvedConstraint) + else if (!isCallVirt || resolvedConstraint) { directCall = true; } @@ -1646,13 +1647,36 @@ private void ceeInfoGetCallInfo( // Direct calls to abstract methods are not allowed if (targetMethod.IsAbstract && // Compensate for always treating delegates as direct calls above - !(((flags & CORINFO_CALLINFO_FLAGS.CORINFO_CALLINFO_LDFTN) != 0) && ((flags & CORINFO_CALLINFO_FLAGS.CORINFO_CALLINFO_CALLVIRT) != 0) && !resolvedCallVirt)) + !(((flags & CORINFO_CALLINFO_FLAGS.CORINFO_CALLINFO_LDFTN) != 0) && isCallVirt && !resolvedCallVirt)) { ThrowHelper.ThrowInvalidProgramException(ExceptionStringID.InvalidProgramCallAbstractMethod, targetMethod); } bool allowInstParam = (flags & CORINFO_CALLINFO_FLAGS.CORINFO_CALLINFO_ALLOWINSTPARAM) != 0; + // If the target method is resolved via constrained static virtual dispatch + // And it requires an instParam, we do not have the generic dictionary infrastructure + // to load the correct generic context arg via EmbedGenericHandle. + // Instead, force the call to go down the CORINFO_CALL_CODE_POINTER code path + // which should have somewhat inferior performance. This should only actually happen in the case + // of shared generic code calling a shared generic implementation method, which should be rare. + // + // An alternative design would be to add a new generic dictionary entry kind to hold the MethodDesc + // of the constrained target instead, and use that in some circumstances; however, implementation of + // that design requires refactoring variuos parts of the JIT interface as well as + // TryResolveConstraintMethodApprox. In particular we would need to be abled to embed a constrained lookup + // via EmbedGenericHandle, as well as decide in TryResolveConstraintMethodApprox if the call can be made + // via a single use of CORINFO_CALL_CODE_POINTER, or would be better done with a CORINFO_CALL + embedded + // constrained generic handle, or if there is a case where we would want to use both a CORINFO_CALL and + // embedded constrained generic handle. Given the current expected high performance use case of this feature + // which is generic numerics which will always resolve to exact valuetypes, it is not expected that + // the complexity involved would be worth the risk. Other scenarios are not expected to be as performance + // sensitive. + if (targetMethod.Signature.IsStatic && constrainedType != null && pResult->exactContextNeedsRuntimeLookup) + { + allowInstParam = false; + } + if (!allowInstParam && canonMethod != null && canonMethod.RequiresInstArg()) { useInstantiatingStub = true; @@ -1699,11 +1723,16 @@ private void ceeInfoGetCallInfo( { pResult->kind = CORINFO_CALL_KIND.CORINFO_CALL_CODE_POINTER; - // For reference types, the constrained type does not affect method resolution - DictionaryEntryKind entryKind = (constrainedType != null && constrainedType.IsValueType + // For reference types, the constrained type does not affect instance virtual method resolution + DictionaryEntryKind entryKind = (constrainedType != null && (constrainedType.IsValueType || !isCallVirt) ? DictionaryEntryKind.ConstrainedMethodEntrySlot : DictionaryEntryKind.MethodEntrySlot); + if (constrainedType != null && originalMethod.Signature.IsStatic && exactType.HasInstantiation) + { + useInstantiatingStub = false; + } + ComputeRuntimeLookupForSharedGenericToken(entryKind, ref pResolvedToken, pConstrainedResolvedToken, originalMethod, ref pResult->codePointerOrStubLookup); } } From 5eea0c500ce1048b4bb26636ddaf092ddf18ef41 Mon Sep 17 00:00:00 2001 From: Tomas Date: Mon, 21 Jun 2021 19:37:52 +0200 Subject: [PATCH 04/11] Use existing virtual method algorithms per Michal's PR feedback I have removed the previously added special virtual method algorithm for static virtual method resolution and instead I added logic for SVM resolution to the existing methods ResolveInterfaceMethodToVirtualMethodOnType and ResolveVariantInterfaceMethodToVirtualMethodOnType I haven't made any functional changes in this commit. Thanks Tomas --- .../Common/MetadataVirtualMethodAlgorithm.cs | 17 ++++++++++++++--- .../TypeSystem/Common/TypeSystemHelpers.cs | 5 ----- .../TypeSystem/Common/VirtualMethodAlgorithm.cs | 2 -- .../JitInterface/CorInfoImpl.ReadyToRun.cs | 2 +- 4 files changed, 15 insertions(+), 11 deletions(-) diff --git a/src/coreclr/tools/Common/TypeSystem/Common/MetadataVirtualMethodAlgorithm.cs b/src/coreclr/tools/Common/TypeSystem/Common/MetadataVirtualMethodAlgorithm.cs index c7d0d4f82011ec..4bccb9f43bebe5 100644 --- a/src/coreclr/tools/Common/TypeSystem/Common/MetadataVirtualMethodAlgorithm.cs +++ b/src/coreclr/tools/Common/TypeSystem/Common/MetadataVirtualMethodAlgorithm.cs @@ -588,6 +588,11 @@ public override MethodDesc ResolveVariantInterfaceMethodToVirtualMethodOnType(Me // See current interface call resolution for details on how that happens. private static MethodDesc ResolveInterfaceMethodToVirtualMethodOnType(MethodDesc interfaceMethod, MetadataType currentType) { + if (interfaceMethod.Signature.IsStatic) + { + return ResolveStaticVirtualMethod(interfaceMethod, currentType, allowVariantMatches: false); + } + if (currentType.IsInterface) return null; @@ -657,6 +662,11 @@ private static MethodDesc ResolveInterfaceMethodToVirtualMethodOnType(MethodDesc public static MethodDesc ResolveVariantInterfaceMethodToVirtualMethodOnType(MethodDesc interfaceMethod, MetadataType currentType) { + if (interfaceMethod.Signature.IsStatic) + { + return ResolveStaticVirtualMethod(interfaceMethod, currentType, allowVariantMatches: true); + } + MetadataType interfaceType = (MetadataType)interfaceMethod.OwningType; bool foundInterface = IsInterfaceImplementedOnType(currentType, interfaceType); MethodDesc implMethod; @@ -830,13 +840,14 @@ public static IEnumerable EnumAllVirtualSlots(MetadataType type) /// /// Try to resolve a given virtual static interface method on a given constrained type and its base types. /// - /// Type to attempt virtual static method resolution on - /// Interface declaring the method /// Interface method to resolve + /// Type to attempt virtual static method resolution on /// True when variant matches are allowed /// MethodDesc of the resolved virtual static method, null when not found (runtime lookup must be used) - public override MethodDesc ResolveInterfaceMethodToStaticVirtualMethodOnType(MethodDesc interfaceMethod, TypeDesc interfaceType, TypeDesc currentType, bool allowVariantMatches) + private static MethodDesc ResolveStaticVirtualMethod(MethodDesc interfaceMethod, TypeDesc currentType, bool allowVariantMatches) { + TypeDesc interfaceType = interfaceMethod.OwningType; + // Search for match on a per-level in the type hierarchy for (TypeDesc typeToCheck = currentType; typeToCheck != null; typeToCheck = typeToCheck.BaseType) { diff --git a/src/coreclr/tools/Common/TypeSystem/Common/TypeSystemHelpers.cs b/src/coreclr/tools/Common/TypeSystem/Common/TypeSystemHelpers.cs index 2a734bdec6c24f..3732586a45e75a 100644 --- a/src/coreclr/tools/Common/TypeSystem/Common/TypeSystemHelpers.cs +++ b/src/coreclr/tools/Common/TypeSystem/Common/TypeSystemHelpers.cs @@ -293,11 +293,6 @@ public static DefaultInterfaceMethodResolution ResolveInterfaceMethodToDefaultIm return type.Context.GetVirtualMethodAlgorithmForType(type).ResolveInterfaceMethodToDefaultImplementationOnType(interfaceMethod, type, out implMethod); } - public static MethodDesc ResolveInterfaceMethodToStaticVirtualMethodOnType(this TypeDesc type, MethodDesc interfaceMethod, TypeDesc interfaceType, bool allowVariantMatches) - { - return type.Context.GetVirtualMethodAlgorithmForType(type).ResolveInterfaceMethodToStaticVirtualMethodOnType(interfaceMethod, interfaceType, type, allowVariantMatches); - } - /// /// Resolves a virtual method call. /// diff --git a/src/coreclr/tools/Common/TypeSystem/Common/VirtualMethodAlgorithm.cs b/src/coreclr/tools/Common/TypeSystem/Common/VirtualMethodAlgorithm.cs index 85a1d78867bc88..e13e61d3862da6 100644 --- a/src/coreclr/tools/Common/TypeSystem/Common/VirtualMethodAlgorithm.cs +++ b/src/coreclr/tools/Common/TypeSystem/Common/VirtualMethodAlgorithm.cs @@ -24,8 +24,6 @@ public abstract class VirtualMethodAlgorithm public abstract MethodDesc ResolveVariantInterfaceMethodToVirtualMethodOnType(MethodDesc interfaceMethod, TypeDesc currentType); - public abstract MethodDesc ResolveInterfaceMethodToStaticVirtualMethodOnType(MethodDesc interfaceMethod, TypeDesc exactInterfaceType, TypeDesc currentType, bool allowVariantMatches); - public abstract DefaultInterfaceMethodResolution ResolveInterfaceMethodToDefaultImplementationOnType(MethodDesc interfaceMethod, TypeDesc currentType, out MethodDesc impl); /// diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs index ea4d7198365993..80e20e0d05724c 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs @@ -1492,7 +1492,7 @@ private void ceeInfoGetCallInfo( { if (_compilation.AllowCompileTimeStaticVirtualMethodResolution(constrainedType, originalMethod, exactType)) { - directMethod = constrainedType.ResolveInterfaceMethodToStaticVirtualMethodOnType(originalMethod, exactType, allowVariantMatches: true); + directMethod = constrainedType.ResolveVariantInterfaceMethodToVirtualMethodOnType(originalMethod); } else { From b4e78cf12bb151004baf904e642d75ae6503afee Mon Sep 17 00:00:00 2001 From: Tomas Date: Mon, 21 Jun 2021 19:56:56 +0200 Subject: [PATCH 05/11] Remove the superfluous interfaceType parameter from DevirtualizationManager --- .../Common/Compiler/DevirtualizationManager.cs | 15 +++++++-------- .../ReadyToRun/DevirtualizationManager.cs | 14 +++++++------- .../Compiler/ReadyToRunCodegenCompilation.cs | 4 ++-- .../JitInterface/CorInfoImpl.ReadyToRun.cs | 2 +- 4 files changed, 17 insertions(+), 18 deletions(-) diff --git a/src/coreclr/tools/Common/Compiler/DevirtualizationManager.cs b/src/coreclr/tools/Common/Compiler/DevirtualizationManager.cs index e9e530bee16b7b..e17e772c072dbe 100644 --- a/src/coreclr/tools/Common/Compiler/DevirtualizationManager.cs +++ b/src/coreclr/tools/Common/Compiler/DevirtualizationManager.cs @@ -208,20 +208,19 @@ protected virtual MethodDesc ResolveVirtualMethod(MethodDesc declMethod, DefType /// /// Type constraint in the static virtual method /// Static virtual interface method to resolve - /// Exact interface type for the static virtual method - public bool AllowCompileTimeStaticVirtualMethodResolution(TypeDesc currentType, MethodDesc interfaceMethod, TypeDesc interfaceType) + public bool AllowCompileTimeStaticVirtualMethodResolution(TypeDesc currentType, MethodDesc interfaceMethod) { Debug.Assert(interfaceMethod.IsVirtual); Debug.Assert(interfaceMethod.Signature.IsStatic); - Debug.Assert(interfaceType.IsInterface); + Debug.Assert(interfaceMethod.OwningType.IsInterface); Debug.Assert(!currentType.IsInterface); - return AllowCompileTimeStaticVirtualMethodResolution(currentType.GetClosestDefType(), interfaceMethod, interfaceType); + return AllowCompileTimeStaticVirtualMethodResolution(currentType.GetClosestDefType(), interfaceMethod); } - protected virtual bool AllowCompileTimeStaticVirtualMethodResolution(DefType currentType, MethodDesc interfaceMethod, TypeDesc interfaceType) + protected virtual bool AllowCompileTimeStaticVirtualMethodResolution(DefType currentType, MethodDesc interfaceMethod) { - if (interfaceMethod.IsSharedByGenericInstantiations || interfaceType.IsCanonicalSubtype(CanonicalFormKind.Any)) + if (interfaceMethod.IsSharedByGenericInstantiations || interfaceMethod.OwningType.IsCanonicalSubtype(CanonicalFormKind.Any)) { return false; } @@ -230,8 +229,8 @@ protected virtual bool AllowCompileTimeStaticVirtualMethodResolution(DefType cur // we cannot exactly compute a target method result, as even if there is an exact match in the type hierarchy // it isn't guaranteed that we will always find the right result, as we may find a match on a base type when we should find the match // on a more derived type. - TypeDesc interfaceTypeCanonical = interfaceType.ConvertToCanonForm(CanonicalFormKind.Specific); - if (interfaceType != interfaceTypeCanonical) + TypeDesc interfaceTypeCanonical = interfaceMethod.OwningType.ConvertToCanonForm(CanonicalFormKind.Specific); + if (interfaceMethod.OwningType != interfaceTypeCanonical) { foreach (DefType runtimeInterface in currentType.RuntimeInterfaces) { diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/DevirtualizationManager.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/DevirtualizationManager.cs index bc6dc728bd13aa..b0c995d16d3763 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/DevirtualizationManager.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/DevirtualizationManager.cs @@ -219,21 +219,21 @@ private enum SVMResolutionEligibility : byte NotApplicable, } - protected override bool AllowCompileTimeStaticVirtualMethodResolution(DefType currentType, MethodDesc interfaceMethod, TypeDesc interfaceType) + protected override bool AllowCompileTimeStaticVirtualMethodResolution(DefType currentType, MethodDesc interfaceMethod) { - if (!base.AllowCompileTimeStaticVirtualMethodResolution(currentType, interfaceMethod, interfaceType)) + if (!base.AllowCompileTimeStaticVirtualMethodResolution(currentType, interfaceMethod)) { return false; } - return AllowCompileTimeStaticVirtualMethodResolutionRecursive(currentType, interfaceMethod, interfaceType) != SVMResolutionEligibility.Forbid; + return AllowCompileTimeStaticVirtualMethodResolutionRecursive(currentType, interfaceMethod) != SVMResolutionEligibility.Forbid; } - private SVMResolutionEligibility AllowCompileTimeStaticVirtualMethodResolutionRecursive(DefType currentType, MethodDesc interfaceMethod, TypeDesc interfaceType) + private SVMResolutionEligibility AllowCompileTimeStaticVirtualMethodResolutionRecursive(DefType currentType, MethodDesc interfaceMethod) { if (currentType.HasBaseType) { - SVMResolutionEligibility allowParent = AllowCompileTimeStaticVirtualMethodResolutionRecursive(currentType.BaseType, interfaceMethod, interfaceType); + SVMResolutionEligibility allowParent = AllowCompileTimeStaticVirtualMethodResolutionRecursive(currentType.BaseType, interfaceMethod); if (allowParent == SVMResolutionEligibility.Forbid) { return SVMResolutionEligibility.Forbid; @@ -246,9 +246,9 @@ private SVMResolutionEligibility AllowCompileTimeStaticVirtualMethodResolutionRe foreach (DefType runtimeInterface in currentType.RuntimeInterfaces) { - if (runtimeInterface == interfaceType) + if (runtimeInterface == interfaceMethod.OwningType) { - return _compilationModuleGroup.VersionsWithType(interfaceType) ? SVMResolutionEligibility.Allow : SVMResolutionEligibility.Forbid; + return _compilationModuleGroup.VersionsWithType(interfaceMethod.OwningType) ? SVMResolutionEligibility.Allow : SVMResolutionEligibility.Forbid; } } diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilation.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilation.cs index db8c53b709f41c..6d13cdac3e8fc8 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilation.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilation.cs @@ -128,9 +128,9 @@ public MethodDesc ResolveVirtualMethod(MethodDesc declMethod, TypeDesc implType, return _devirtualizationManager.ResolveVirtualMethod(declMethod, implType, out devirtualizationDetail); } - public bool AllowCompileTimeStaticVirtualMethodResolution(TypeDesc implType, MethodDesc interfaceMethod, TypeDesc interfaceType) + public bool AllowCompileTimeStaticVirtualMethodResolution(TypeDesc implType, MethodDesc interfaceMethod) { - return _devirtualizationManager.AllowCompileTimeStaticVirtualMethodResolution(implType, interfaceMethod, interfaceType); + return _devirtualizationManager.AllowCompileTimeStaticVirtualMethodResolution(implType, interfaceMethod); } public bool IsModuleInstrumented(ModuleDesc module) diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs index 80e20e0d05724c..c9bfa03023a761 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs @@ -1490,7 +1490,7 @@ private void ceeInfoGetCallInfo( MethodDesc directMethod; if (originalMethod.Signature.IsStatic) { - if (_compilation.AllowCompileTimeStaticVirtualMethodResolution(constrainedType, originalMethod, exactType)) + if (_compilation.AllowCompileTimeStaticVirtualMethodResolution(constrainedType, originalMethod)) { directMethod = constrainedType.ResolveVariantInterfaceMethodToVirtualMethodOnType(originalMethod); } From 118e3fa04ef0d5cf00382331b7d48e3779ba41eb Mon Sep 17 00:00:00 2001 From: Tomas Date: Wed, 30 Jun 2021 15:17:28 +0200 Subject: [PATCH 06/11] Address additional Michal's and David's feedback; all tests pass --- .../Common/MetadataVirtualMethodAlgorithm.cs | 103 ++++++++++-------- .../TypeSystem/Common/TypeSystemHelpers.cs | 5 + .../Common/VirtualMethodAlgorithm.cs | 3 + .../JitInterface/CorInfoImpl.ReadyToRun.cs | 12 +- 4 files changed, 69 insertions(+), 54 deletions(-) diff --git a/src/coreclr/tools/Common/TypeSystem/Common/MetadataVirtualMethodAlgorithm.cs b/src/coreclr/tools/Common/TypeSystem/Common/MetadataVirtualMethodAlgorithm.cs index 4bccb9f43bebe5..ee3f3830ce3b00 100644 --- a/src/coreclr/tools/Common/TypeSystem/Common/MetadataVirtualMethodAlgorithm.cs +++ b/src/coreclr/tools/Common/TypeSystem/Common/MetadataVirtualMethodAlgorithm.cs @@ -572,6 +572,11 @@ public override MethodDesc ResolveVariantInterfaceMethodToVirtualMethodOnType(Me return ResolveVariantInterfaceMethodToVirtualMethodOnType(interfaceMethod, (MetadataType)currentType); } + public override MethodDesc ResolveVariantInterfaceMethodToStaticVirtualMethodOnType(MethodDesc interfaceMethod, TypeDesc currentType, Func inVersionBubble) + { + return ResolveVariantInterfaceMethodToStaticVirtualMethodOnType(interfaceMethod, (MetadataType)currentType, inVersionBubble); + } + //////////////////////// INTERFACE RESOLUTION //Interface function resolution // Interface function resolution follows the following rules @@ -588,11 +593,6 @@ public override MethodDesc ResolveVariantInterfaceMethodToVirtualMethodOnType(Me // See current interface call resolution for details on how that happens. private static MethodDesc ResolveInterfaceMethodToVirtualMethodOnType(MethodDesc interfaceMethod, MetadataType currentType) { - if (interfaceMethod.Signature.IsStatic) - { - return ResolveStaticVirtualMethod(interfaceMethod, currentType, allowVariantMatches: false); - } - if (currentType.IsInterface) return null; @@ -662,11 +662,6 @@ private static MethodDesc ResolveInterfaceMethodToVirtualMethodOnType(MethodDesc public static MethodDesc ResolveVariantInterfaceMethodToVirtualMethodOnType(MethodDesc interfaceMethod, MetadataType currentType) { - if (interfaceMethod.Signature.IsStatic) - { - return ResolveStaticVirtualMethod(interfaceMethod, currentType, allowVariantMatches: true); - } - MetadataType interfaceType = (MetadataType)interfaceMethod.OwningType; bool foundInterface = IsInterfaceImplementedOnType(currentType, interfaceType); MethodDesc implMethod; @@ -842,62 +837,81 @@ public static IEnumerable EnumAllVirtualSlots(MetadataType type) /// /// Interface method to resolve /// Type to attempt virtual static method resolution on - /// True when variant matches are allowed /// MethodDesc of the resolved virtual static method, null when not found (runtime lookup must be used) - private static MethodDesc ResolveStaticVirtualMethod(MethodDesc interfaceMethod, TypeDesc currentType, bool allowVariantMatches) + public static MethodDesc ResolveVariantInterfaceMethodToStaticVirtualMethodOnType(MethodDesc interfaceMethod, MetadataType currentType, Func inVersionBubble) { TypeDesc interfaceType = interfaceMethod.OwningType; + if (!inVersionBubble(interfaceType)) + { + return null; + } // Search for match on a per-level in the type hierarchy for (TypeDesc typeToCheck = currentType; typeToCheck != null; typeToCheck = typeToCheck.BaseType) { - MethodDesc resolvedMethodOnType = TryResolveVirtualStaticMethodOnThisType(typeToCheck, interfaceType, interfaceMethod); + if (!inVersionBubble(typeToCheck)) + { + return null; + } + + MethodDesc resolvedMethodOnType = TryResolveVirtualStaticMethodOnThisType(typeToCheck, interfaceMethod); if (resolvedMethodOnType != null) { return resolvedMethodOnType; } - if (interfaceType.HasVariance /* TODO || interfaceType.HasTypeEquivalence */) + // Variant interface dispatch + foreach (DefType runtimeInterfaceType in typeToCheck.RuntimeInterfaces) { - // Variant interface dispatch - foreach (DefType runtimeInterfaceType in typeToCheck.RuntimeInterfaces) + if (runtimeInterfaceType == interfaceType) { - if (runtimeInterfaceType == interfaceType) - { - // This is the variant interface check logic, skip this - continue; - } + // This is the variant interface check logic, skip this + continue; + } - if (!runtimeInterfaceType.HasSameTypeDefinition(interfaceType)) + if (!runtimeInterfaceType.HasSameTypeDefinition(interfaceType)) + { + // Variance matches require a typedef match + // Equivalence isn't sufficient, and is uninteresting as equivalent interfaces cannot have static virtuals. + continue; + } + + if (runtimeInterfaceType.CanCastTo(interfaceType)) + { + if (!inVersionBubble(runtimeInterfaceType)) { - // Variance matches require a typedef match - // Equivalence isn't sufficient, and is uninteresting as equivalent interfaces cannot have static virtuals. - continue; + // Fail the resolution if a candidate variant interface match is outside of the version bubble + return null; } - bool equivalentOrVariantCompatible; + // Attempt to resolve on variance matched interface + MethodDesc runtimeInterfaceMethod = TryResolveInterfaceMethodOnVariantCompatibleInterface(runtimeInterfaceType, interfaceMethod); - if (allowVariantMatches) + if (runtimeInterfaceMethod != null) { - equivalentOrVariantCompatible = runtimeInterfaceType.CanCastTo(interfaceType); + resolvedMethodOnType = TryResolveVirtualStaticMethodOnThisType(typeToCheck, runtimeInterfaceMethod); } - else + if (resolvedMethodOnType != null) { - // When performing override checking to ensure that a concrete type is valid, require the implementation - // actually implement the exact or equivalent interface. - equivalentOrVariantCompatible = false; // TODO: runtimeInterfaceType.IsEquivalentTo(interfaceType); + return resolvedMethodOnType; } + } + } + } + return null; + } - if (equivalentOrVariantCompatible) - { - // Variant or equivalent matching interface found - // Attempt to resolve on variance matched interface - resolvedMethodOnType = TryResolveVirtualStaticMethodOnThisType(typeToCheck, runtimeInterfaceType, interfaceMethod); - if (resolvedMethodOnType != null) - { - return resolvedMethodOnType; - } - } + private static MethodDesc TryResolveInterfaceMethodOnVariantCompatibleInterface(TypeDesc compatibleInterfaceType, MethodDesc interfaceMethod) + { + Debug.Assert(compatibleInterfaceType.CanCastTo(interfaceMethod.OwningType)); + if (compatibleInterfaceType is MetadataType mdType) + { + foreach (MethodDesc runtimeInterfaceMethod in mdType.GetVirtualMethods()) + { + if (runtimeInterfaceMethod.Name == interfaceMethod.Name && + runtimeInterfaceMethod.Signature == interfaceMethod.Signature) + { + return runtimeInterfaceMethod; } } } @@ -911,14 +925,13 @@ private static MethodDesc ResolveStaticVirtualMethod(MethodDesc interfaceMethod, /// Interface declaring the method /// Method to resolve /// MethodDesc of the resolved method or null when not found (runtime lookup must be used) - private static MethodDesc TryResolveVirtualStaticMethodOnThisType(TypeDesc constrainedType, TypeDesc interfaceType, MethodDesc interfaceMethod) + private static MethodDesc TryResolveVirtualStaticMethodOnThisType(TypeDesc constrainedType, MethodDesc interfaceMethod) { if (constrainedType is MetadataType mdType) { foreach (MethodImplRecord methodImpl in mdType.FindMethodsImplWithMatchingDeclName(interfaceMethod.Name) ?? Array.Empty()) { - if (methodImpl.Decl.OwningType == interfaceType && - methodImpl.Decl == interfaceMethod) + if (methodImpl.Decl == interfaceMethod) { MethodDesc resolvedMethodImpl = methodImpl.Body; if (resolvedMethodImpl.OwningType != mdType) diff --git a/src/coreclr/tools/Common/TypeSystem/Common/TypeSystemHelpers.cs b/src/coreclr/tools/Common/TypeSystem/Common/TypeSystemHelpers.cs index 3732586a45e75a..5e8fe65669fd3a 100644 --- a/src/coreclr/tools/Common/TypeSystem/Common/TypeSystemHelpers.cs +++ b/src/coreclr/tools/Common/TypeSystem/Common/TypeSystemHelpers.cs @@ -288,6 +288,11 @@ public static MethodDesc ResolveVariantInterfaceMethodToVirtualMethodOnType(this return type.Context.GetVirtualMethodAlgorithmForType(type).ResolveVariantInterfaceMethodToVirtualMethodOnType(interfaceMethod, type); } + public static MethodDesc ResolveVariantInterfaceMethodToStaticVirtualMethodOnType(this TypeDesc type, MethodDesc interfaceMethod, Func inVersionBubble) + { + return type.Context.GetVirtualMethodAlgorithmForType(type).ResolveVariantInterfaceMethodToStaticVirtualMethodOnType(interfaceMethod, type, inVersionBubble); + } + public static DefaultInterfaceMethodResolution ResolveInterfaceMethodToDefaultImplementationOnType(this TypeDesc type, MethodDesc interfaceMethod, out MethodDesc implMethod) { return type.Context.GetVirtualMethodAlgorithmForType(type).ResolveInterfaceMethodToDefaultImplementationOnType(interfaceMethod, type, out implMethod); diff --git a/src/coreclr/tools/Common/TypeSystem/Common/VirtualMethodAlgorithm.cs b/src/coreclr/tools/Common/TypeSystem/Common/VirtualMethodAlgorithm.cs index e13e61d3862da6..19cb2ee7ad12a5 100644 --- a/src/coreclr/tools/Common/TypeSystem/Common/VirtualMethodAlgorithm.cs +++ b/src/coreclr/tools/Common/TypeSystem/Common/VirtualMethodAlgorithm.cs @@ -1,6 +1,7 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System; using System.Collections.Generic; namespace Internal.TypeSystem @@ -24,6 +25,8 @@ public abstract class VirtualMethodAlgorithm public abstract MethodDesc ResolveVariantInterfaceMethodToVirtualMethodOnType(MethodDesc interfaceMethod, TypeDesc currentType); + public abstract MethodDesc ResolveVariantInterfaceMethodToStaticVirtualMethodOnType(MethodDesc interfaceMethod, TypeDesc currentType, Func inVersionBubble); + public abstract DefaultInterfaceMethodResolution ResolveInterfaceMethodToDefaultImplementationOnType(MethodDesc interfaceMethod, TypeDesc currentType, out MethodDesc impl); /// diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs index c9bfa03023a761..3e1bcb4ae26c90 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs @@ -943,7 +943,8 @@ private ModuleToken HandleToModuleToken(ref CORINFO_RESOLVED_TOKEN pResolvedToke || (pResolvedToken.tokenType == CorInfoTokenKind.CORINFO_TOKENKIND_DevirtualizedMethod) || methodDesc.IsPInvoke)) { - if ((CorTokenType)(unchecked((uint)pResolvedToken.token) & 0xFF000000u) == CorTokenType.mdtMethodDef && + CorTokenType corTokenType = (CorTokenType)(unchecked((uint)pResolvedToken.token) & 0xFF000000u); + if ((corTokenType == CorTokenType.mdtMethodDef || corTokenType == CorTokenType.mdtMemberRef) && methodDesc?.GetTypicalMethodDefinition() is EcmaMethod ecmaMethod) { mdToken token = (mdToken)MetadataTokens.GetToken(ecmaMethod.Handle); @@ -1490,14 +1491,7 @@ private void ceeInfoGetCallInfo( MethodDesc directMethod; if (originalMethod.Signature.IsStatic) { - if (_compilation.AllowCompileTimeStaticVirtualMethodResolution(constrainedType, originalMethod)) - { - directMethod = constrainedType.ResolveVariantInterfaceMethodToVirtualMethodOnType(originalMethod); - } - else - { - directMethod = null; - } + directMethod = constrainedType.ResolveVariantInterfaceMethodToStaticVirtualMethodOnType(originalMethod, _compilation.CompilationModuleGroup.VersionsWithType); forceUseRuntimeLookup = (directMethod == null); } else From 91e18055e8fb13340328615767329ebb1f9aa7e2 Mon Sep 17 00:00:00 2001 From: Tomas Date: Fri, 2 Jul 2021 09:15:55 +0200 Subject: [PATCH 07/11] Several JIT interface fixes for static virtual methods --- .../JitInterface/CorInfoImpl.ReadyToRun.cs | 32 ++++++++++++++----- 1 file changed, 24 insertions(+), 8 deletions(-) diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs index 3e1bcb4ae26c90..648350eec21b78 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs @@ -1450,6 +1450,8 @@ private void ceeInfoGetCallInfo( callerModule = ((EcmaMethod)callerMethod.GetTypicalMethodDefinition()).Module; bool isCallVirt = (flags & CORINFO_CALLINFO_FLAGS.CORINFO_CALLINFO_CALLVIRT) != 0; + bool isLdftn = (flags & CORINFO_CALLINFO_FLAGS.CORINFO_CALLINFO_LDFTN) != 0; + bool isStaticVirtual = (originalMethod.Signature.IsStatic && originalMethod.IsVirtual); // Spec says that a callvirt lookup ignores static methods. Since static methods // can't have the exact same signature as instance methods, a lookup that found @@ -1489,7 +1491,7 @@ private void ceeInfoGetCallInfo( } MethodDesc directMethod; - if (originalMethod.Signature.IsStatic) + if (isStaticVirtual) { directMethod = constrainedType.ResolveVariantInterfaceMethodToStaticVirtualMethodOnType(originalMethod, _compilation.CompilationModuleGroup.VersionsWithType); forceUseRuntimeLookup = (directMethod == null); @@ -1520,6 +1522,10 @@ private void ceeInfoGetCallInfo( exactType = constrainedType; } + else if (isStaticVirtual) + { + pResult->thisTransform = CORINFO_THIS_TRANSFORM.CORINFO_NO_THIS_TRANSFORM; + } else if (constrainedType.IsValueType) { pResult->thisTransform = CORINFO_THIS_TRANSFORM.CORINFO_BOX_THIS; @@ -1569,12 +1575,15 @@ private void ceeInfoGetCallInfo( bool resolvedCallVirt = false; bool callVirtCrossingVersionBubble = false; - if ((flags & CORINFO_CALLINFO_FLAGS.CORINFO_CALLINFO_LDFTN) != 0) + if (isLdftn) { directCall = true; } - else - if (targetMethod.Signature.IsStatic) + else if (isStaticVirtual && !resolvedConstraint) + { + // Don't use direct calls for static virtual method calls unresolved at compile time + } + else if (targetMethod.Signature.IsStatic) { // Static methods are always direct calls directCall = true; @@ -1638,10 +1647,12 @@ private void ceeInfoGetCallInfo( if (directCall) { + bool isVirtualBehaviorUnresolved = (isCallVirt && !resolvedCallVirt || isStaticVirtual && !resolvedConstraint); + // Direct calls to abstract methods are not allowed if (targetMethod.IsAbstract && // Compensate for always treating delegates as direct calls above - !(((flags & CORINFO_CALLINFO_FLAGS.CORINFO_CALLINFO_LDFTN) != 0) && isCallVirt && !resolvedCallVirt)) + !(isLdftn && isVirtualBehaviorUnresolved)) { ThrowHelper.ThrowInvalidProgramException(ExceptionStringID.InvalidProgramCallAbstractMethod, targetMethod); } @@ -1666,7 +1677,7 @@ private void ceeInfoGetCallInfo( // which is generic numerics which will always resolve to exact valuetypes, it is not expected that // the complexity involved would be worth the risk. Other scenarios are not expected to be as performance // sensitive. - if (targetMethod.Signature.IsStatic && constrainedType != null && pResult->exactContextNeedsRuntimeLookup) + if (isStaticVirtual && pResult->exactContextNeedsRuntimeLookup) { allowInstParam = false; } @@ -1722,9 +1733,9 @@ private void ceeInfoGetCallInfo( ? DictionaryEntryKind.ConstrainedMethodEntrySlot : DictionaryEntryKind.MethodEntrySlot); - if (constrainedType != null && originalMethod.Signature.IsStatic && exactType.HasInstantiation) + if (isStaticVirtual && exactType.HasInstantiation) { - useInstantiatingStub = false; + useInstantiatingStub = true; } ComputeRuntimeLookupForSharedGenericToken(entryKind, ref pResolvedToken, pConstrainedResolvedToken, originalMethod, ref pResult->codePointerOrStubLookup); @@ -1748,6 +1759,11 @@ private void ceeInfoGetCallInfo( } pResult->nullInstanceCheck = resolvedCallVirt; } + else if (isStaticVirtual) + { + pResult->kind = CORINFO_CALL_KIND.CORINFO_CALL; + pResult->nullInstanceCheck = false; + } // All virtual calls which take method instantiations must // currently be implemented by an indirect call via a runtime-lookup // function pointer From 9d5c7f57da5a74dc76a637c7539b010b66d7210c Mon Sep 17 00:00:00 2001 From: Tomas Date: Mon, 12 Jul 2021 12:15:42 +0200 Subject: [PATCH 08/11] Don't drop constrained type for SVMs unresolved at compile time With this change, more tests seem to be passing but there seems to be a runtime problem ending up as an assertion: for instance, in TypeHierarchyTests, Scenario 25, TryResolveConstraintMethodApprox resolves the constrained call to BaseScenario25>.Method() which subsequently gets wrapped into an instantiating stub and later crashes GC ref map check which seems to compare the GC ref map of the wrapped method with the original import cell for the constrained method. These naturally don't match as the wrapped canonical method requires the method dictionary argument. I continue investigating this but any insight into what exactly needs fixing here would be more than welcome. Thanks Tomas --- .../JitInterface/CorInfoImpl.ReadyToRun.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs index 648350eec21b78..2ccdafae788d48 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs @@ -939,7 +939,8 @@ private MethodWithToken ComputeMethodWithToken(MethodDesc method, ref CORINFO_RE private ModuleToken HandleToModuleToken(ref CORINFO_RESOLVED_TOKEN pResolvedToken, MethodDesc methodDesc, out object context, ref TypeDesc constrainedType) { - if (methodDesc != null && (_compilation.NodeFactory.CompilationModuleGroup.VersionsWithMethodBody(methodDesc) + if (methodDesc != null && !methodDesc.IsAbstract && + (_compilation.NodeFactory.CompilationModuleGroup.VersionsWithMethodBody(methodDesc) || (pResolvedToken.tokenType == CorInfoTokenKind.CORINFO_TOKENKIND_DevirtualizedMethod) || methodDesc.IsPInvoke)) { From c3248ecc0e7b65eaf7871c986a1c92e86d5d9c6e Mon Sep 17 00:00:00 2001 From: Tomas Date: Mon, 19 Jul 2021 17:29:04 +0200 Subject: [PATCH 09/11] Use instantiating stubs for unresolved SVM import cells --- .../JitInterface/CorInfoImpl.ReadyToRun.cs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs index 2ccdafae788d48..62db8829fe1e0a 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs @@ -1764,6 +1764,16 @@ private void ceeInfoGetCallInfo( { pResult->kind = CORINFO_CALL_KIND.CORINFO_CALL; pResult->nullInstanceCheck = false; + + // Always use an instantiating stub for unresolved constrained SVM calls as we cannot + // always tell at compile time that a given SVM resolves to a method on a generic base + // class and not requesting the instantiating stub makes the runtime transform the + // owning type to its canonical equivalent that would need different codegen + // (supplying the instantiation argument). + if (!resolvedConstraint && !originalMethod.IsSharedByGenericInstantiations) + { + useInstantiatingStub = true; + } } // All virtual calls which take method instantiations must // currently be implemented by an indirect call via a runtime-lookup From 147bf51ac7785fde632b547590774a89f79cb814 Mon Sep 17 00:00:00 2001 From: Tomas Date: Mon, 19 Jul 2021 23:35:27 +0200 Subject: [PATCH 10/11] Simplify the change by applying David's suggestion to not JIT unresolved SVM callers --- .../JitInterface/CorInfoImpl.ReadyToRun.cs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs index 62db8829fe1e0a..4eafab65a9f35b 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs @@ -1495,7 +1495,10 @@ private void ceeInfoGetCallInfo( if (isStaticVirtual) { directMethod = constrainedType.ResolveVariantInterfaceMethodToStaticVirtualMethodOnType(originalMethod, _compilation.CompilationModuleGroup.VersionsWithType); - forceUseRuntimeLookup = (directMethod == null); + if (directMethod == null) + { + throw new RequiresRuntimeJitException(originalMethod.ToString()); + } } else { From 4815ff730894e59100a46c018239e607d741eea0 Mon Sep 17 00:00:00 2001 From: Tomas Date: Mon, 19 Jul 2021 23:37:56 +0200 Subject: [PATCH 11/11] Remove no longer used method AllowCompileTimeStaticVirtualMethodResolution --- .../Compiler/DevirtualizationManager.cs | 42 ------------------ .../ReadyToRun/DevirtualizationManager.cs | 43 ------------------- .../Compiler/ReadyToRunCodegenCompilation.cs | 5 --- 3 files changed, 90 deletions(-) diff --git a/src/coreclr/tools/Common/Compiler/DevirtualizationManager.cs b/src/coreclr/tools/Common/Compiler/DevirtualizationManager.cs index e17e772c072dbe..aba41341a27095 100644 --- a/src/coreclr/tools/Common/Compiler/DevirtualizationManager.cs +++ b/src/coreclr/tools/Common/Compiler/DevirtualizationManager.cs @@ -201,47 +201,5 @@ protected virtual MethodDesc ResolveVirtualMethod(MethodDesc declMethod, DefType return impl; } - - /// - /// Check whether compile-time resolution of static virtual method is possible. This returns false for shared - /// generics where the resolution needs to be deferred to the runtime working with exact instantiation types. - /// - /// Type constraint in the static virtual method - /// Static virtual interface method to resolve - public bool AllowCompileTimeStaticVirtualMethodResolution(TypeDesc currentType, MethodDesc interfaceMethod) - { - Debug.Assert(interfaceMethod.IsVirtual); - Debug.Assert(interfaceMethod.Signature.IsStatic); - Debug.Assert(interfaceMethod.OwningType.IsInterface); - Debug.Assert(!currentType.IsInterface); - - return AllowCompileTimeStaticVirtualMethodResolution(currentType.GetClosestDefType(), interfaceMethod); - } - - protected virtual bool AllowCompileTimeStaticVirtualMethodResolution(DefType currentType, MethodDesc interfaceMethod) - { - if (interfaceMethod.IsSharedByGenericInstantiations || interfaceMethod.OwningType.IsCanonicalSubtype(CanonicalFormKind.Any)) - { - return false; - } - - // Check that there is no implementation of the interface on this type which is the canonical interface for a shared generic. If so, that indicates that - // we cannot exactly compute a target method result, as even if there is an exact match in the type hierarchy - // it isn't guaranteed that we will always find the right result, as we may find a match on a base type when we should find the match - // on a more derived type. - TypeDesc interfaceTypeCanonical = interfaceMethod.OwningType.ConvertToCanonForm(CanonicalFormKind.Specific); - if (interfaceMethod.OwningType != interfaceTypeCanonical) - { - foreach (DefType runtimeInterface in currentType.RuntimeInterfaces) - { - if (interfaceTypeCanonical == runtimeInterface) - { - return false; - } - } - } - - return true; - } } } diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/DevirtualizationManager.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/DevirtualizationManager.cs index b0c995d16d3763..ab85bb84936f71 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/DevirtualizationManager.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/DevirtualizationManager.cs @@ -211,48 +211,5 @@ static TypeDesc FindVersionBubbleEdge(CompilationModuleGroup compilationModuleGr return type; } } - - private enum SVMResolutionEligibility : byte - { - Allow, - Forbid, - NotApplicable, - } - - protected override bool AllowCompileTimeStaticVirtualMethodResolution(DefType currentType, MethodDesc interfaceMethod) - { - if (!base.AllowCompileTimeStaticVirtualMethodResolution(currentType, interfaceMethod)) - { - return false; - } - - return AllowCompileTimeStaticVirtualMethodResolutionRecursive(currentType, interfaceMethod) != SVMResolutionEligibility.Forbid; - } - - private SVMResolutionEligibility AllowCompileTimeStaticVirtualMethodResolutionRecursive(DefType currentType, MethodDesc interfaceMethod) - { - if (currentType.HasBaseType) - { - SVMResolutionEligibility allowParent = AllowCompileTimeStaticVirtualMethodResolutionRecursive(currentType.BaseType, interfaceMethod); - if (allowParent == SVMResolutionEligibility.Forbid) - { - return SVMResolutionEligibility.Forbid; - } - if (allowParent == SVMResolutionEligibility.Allow) - { - return _compilationModuleGroup.VersionsWithType(currentType.BaseType) ? SVMResolutionEligibility.Allow : SVMResolutionEligibility.Forbid; - } - } - - foreach (DefType runtimeInterface in currentType.RuntimeInterfaces) - { - if (runtimeInterface == interfaceMethod.OwningType) - { - return _compilationModuleGroup.VersionsWithType(interfaceMethod.OwningType) ? SVMResolutionEligibility.Allow : SVMResolutionEligibility.Forbid; - } - } - - return SVMResolutionEligibility.NotApplicable; - } } } diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilation.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilation.cs index 6d13cdac3e8fc8..ac7058bf90daec 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilation.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilation.cs @@ -128,11 +128,6 @@ public MethodDesc ResolveVirtualMethod(MethodDesc declMethod, TypeDesc implType, return _devirtualizationManager.ResolveVirtualMethod(declMethod, implType, out devirtualizationDetail); } - public bool AllowCompileTimeStaticVirtualMethodResolution(TypeDesc implType, MethodDesc interfaceMethod) - { - return _devirtualizationManager.AllowCompileTimeStaticVirtualMethodResolution(implType, interfaceMethod); - } - public bool IsModuleInstrumented(ModuleDesc module) { return _modulesBeingInstrumented.Contains(module);