Allow eliminating virtual methods on abstract classes

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/CompilationModuleGroup.Aot.cs

@@ -55,5 +55,11 @@ public partial class CompilationModuleGroup : IInliningPolicy
         /// If true, instance methods will only be generated once their owning type is created.
         /// </summary>
         public abstract bool AllowInstanceMethodOptimization(MethodDesc method);
+
+        /// <summary>
+        /// If true, virtual methods on abstract types will only be generated once a non-abstract derived
+        /// type that doesn't override the virtual method is created.
+        /// </summary>
+        public abstract bool AllowVirtualMethodOnAbstractTypeOptimization(MethodDesc method);
     }
 }

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/EETypeNode.cs

@@ -280,11 +280,25 @@ public sealed override bool HasConditionalStaticDependencies
                 //
                 // The conditional dependencies conditionally add the implementation of the virtual method
                 // if the virtual method is used.
-                foreach (var method in _type.GetClosestDefType().GetAllVirtualMethods())
+                //
+                // We walk the inheritance chain because abstract bases would only add a "tentative"
+                // method body of the implementation that can be trimmed away if no other type uses it.
+                DefType currentType = _type.GetClosestDefType();
+                while (currentType != null)
                 {
-                    // Generic virtual methods are tracked by an orthogonal mechanism.
-                    if (!method.HasInstantiation)
-                        return true;
+                    if (currentType == _type || (currentType is MetadataType mdType && mdType.IsAbstract))
+                    {
+                        foreach (var method in currentType.GetAllVirtualMethods())
+                        {
+                            // Abstract methods don't have a body associated with it so there's no conditional
+                            // dependency to add.
+                            // Generic virtual methods are tracked by an orthogonal mechanism.
+                            if (!method.IsAbstract && !method.HasInstantiation)
+                                return true;
+                        }
+                    }
+
+                    currentType = currentType.BaseType;
                 }
 
                 // If the type implements at least one interface, calls against that interface could result in this type's
@@ -342,17 +356,43 @@ public sealed override IEnumerable<CombinedDependencyListEntry> GetConditionalSt
 
             if (needsDependenciesForVirtualMethodImpls)
             {
+                bool isNonInterfaceAbstractType = !defType.IsInterface && ((MetadataType)defType).IsAbstract;
+
                 foreach (MethodDesc decl in defType.EnumAllVirtualSlots())
                 {
                     // Generic virtual methods are tracked by an orthogonal mechanism.
                     if (decl.HasInstantiation)
                         continue;
 
                     MethodDesc impl = defType.FindVirtualFunctionTargetMethodOnObjectType(decl);
-                    if (impl.OwningType == defType && !impl.IsAbstract)
+                    bool implOwnerIsAbstract = ((MetadataType)impl.OwningType).IsAbstract;
+
+                    // We add a conditional dependency in two situations:
+                    // 1. The implementation is on this type. This is pretty obvious.
+                    // 2. The implementation comes from an abstract base type. We do this
+                    //    because abstract types only request a TentativeMethodEntrypoint of the implementation.
+                    //    The actual method body of this entrypoint might still be trimmed away.
+                    //    We don't need to do this for implementations from non-abstract bases since
+                    //    non-abstract types will create a hard conditional reference to their virtual
+                    //    method implementations.
+                    //
+                    // We also skip abstract methods since they don't have a body to refer to.
+                    if ((impl.OwningType == defType || implOwnerIsAbstract) && !impl.IsAbstract)
                     {
                         MethodDesc canonImpl = impl.GetCanonMethodTarget(CanonicalFormKind.Specific);
-                        IMethodNode implNode = factory.MethodEntrypoint(canonImpl, impl.OwningType.IsValueType);
+
+                        // If this is an abstract type, only request a tentative entrypoint (whose body
+                        // might just be stubbed out). This lets us avoid generating method bodies for
+                        // virtual method on abstract types that are overriden in all their children.
+                        //
+                        // We don't do this if the method can be placed in the sealed vtable since
+                        // those can never be overriden by children anyway.
+                        bool canUseTentativeMethod = isNonInterfaceAbstractType
+                            && !decl.CanMethodBeInSealedVTable()
+                            && factory.CompilationModuleGroup.AllowVirtualMethodOnAbstractTypeOptimization(canonImpl);
+                        IMethodNode implNode = canUseTentativeMethod ?
+                            factory.TentativeMethodEntrypoint(canonImpl, impl.OwningType.IsValueType) :
+                            factory.MethodEntrypoint(canonImpl, impl.OwningType.IsValueType);
                         result.Add(new CombinedDependencyListEntry(implNode, factory.VirtualMethodUse(decl), "Virtual method"));
                     }
 
@@ -932,7 +972,20 @@ private void OutputVirtualSlots(NodeFactory factory, ref ObjectDataBuilder objDa
                 if (!implMethod.IsAbstract)
                 {
                     MethodDesc canonImplMethod = implMethod.GetCanonMethodTarget(CanonicalFormKind.Specific);
-                    objData.EmitPointerReloc(factory.MethodEntrypoint(canonImplMethod, implMethod.OwningType.IsValueType));
+
+                    // If the type we're generating now is abstract, and the implementation comes from an abstract type,
+                    // only use a tentative method entrypoint that can have its body replaced by a throwing stub
+                    // if no "hard" reference to that entrypoint exists in the program.
+                    // This helps us to eliminate method bodies for virtual methods on abstract types that are fully overriden
+                    // in the children of that abstract type.
+                    bool canUseTentativeEntrypoint = implType is MetadataType mdImplType && mdImplType.IsAbstract && !mdImplType.IsInterface
+                        && implMethod.OwningType is MetadataType mdImplMethodType && mdImplMethodType.IsAbstract
+                        && factory.CompilationModuleGroup.AllowVirtualMethodOnAbstractTypeOptimization(canonImplMethod);
+
+                    IMethodNode implSymbol = canUseTentativeEntrypoint ?
+                        factory.TentativeMethodEntrypoint(canonImplMethod, implMethod.OwningType.IsValueType) :
+                        factory.MethodEntrypoint(canonImplMethod, implMethod.OwningType.IsValueType);
+                    objData.EmitPointerReloc(implSymbol);
                 }
                 else
                 {

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/NodeFactory.cs

@@ -255,6 +255,13 @@ private void CreateNodeCaches()
 
             _methodEntrypoints = new MethodEntrypointHashtable(this);
 
+            _tentativeMethodEntrypoints = new NodeCache<MethodDesc, IMethodNode>((MethodDesc method) =>
+            {
+                IMethodNode entrypoint = MethodEntrypoint(method, unboxingStub: false);
+                return new TentativeMethodNode(entrypoint is TentativeMethodNode tentative ?
+                    tentative.RealBody : (IMethodBodyNode)entrypoint);
+            });
+
             _tentativeMethods = new NodeCache<MethodDesc, TentativeInstanceMethodNode>(method =>
             {
                 return new TentativeInstanceMethodNode((IMethodBodyNode)MethodEntrypoint(method));
@@ -849,6 +856,15 @@ public IMethodNode MethodEntrypoint(MethodDesc method, bool unboxingStub = false
             return _methodEntrypoints.GetOrCreateValue(method);
         }
 
+        protected NodeCache<MethodDesc, IMethodNode> _tentativeMethodEntrypoints;
+
+        public IMethodNode TentativeMethodEntrypoint(MethodDesc method, bool unboxingStub = false)
+        {
+            // Didn't implement unboxing stubs for now. Would need to pass down the flag.
+            Debug.Assert(!unboxingStub);
+            return _tentativeMethodEntrypoints.GetOrAdd(method);
+        }
+
         private NodeCache<MethodDesc, TentativeInstanceMethodNode> _tentativeMethods;
         public IMethodNode MethodEntrypointOrTentativeMethod(MethodDesc method, bool unboxingStub = false)
         {

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ILScanner.cs

@@ -442,7 +442,9 @@ public ScannedDevirtualizationManager(ImmutableArray<DependencyNodeCore<NodeFact
                             }
 
                             TypeDesc canonType = type.ConvertToCanonForm(CanonicalFormKind.Specific);
-                            _canonConstructedTypes.Add(canonType.GetClosestDefType());
+
+                            if (!canonType.IsDefType || !((MetadataType)canonType).IsAbstract)
+                                _canonConstructedTypes.Add(canonType.GetClosestDefType());
 
                             TypeDesc baseType = canonType.BaseType;
                             bool added = true;

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/MultiFileCompilationModuleGroup.cs

@@ -126,5 +126,13 @@ public override bool AllowInstanceMethodOptimization(MethodDesc method)
             }
             return false;
         }
+
+        public override bool AllowVirtualMethodOnAbstractTypeOptimization(MethodDesc method)
+        {
+            // Not really safe to do this since we need to assume IgnoreAccessChecks
+            // and we wouldn't know all derived types when compiling methods on the type
+            // that introduces this method.
+            return false;
+        }
     }
 }

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/SingleFileCompilationModuleGroup.cs

@@ -75,5 +75,10 @@ public override bool AllowInstanceMethodOptimization(MethodDesc method)
         {
             return true;
         }
+
+        public override bool AllowVirtualMethodOnAbstractTypeOptimization(MethodDesc method)
+        {
+            return true;
+        }
     }
 }

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/SingleMethodCompilationModuleGroup.cs

@@ -86,5 +86,10 @@ public override bool AllowInstanceMethodOptimization(MethodDesc method)
         {
             return false;
         }
+
+        public override bool AllowVirtualMethodOnAbstractTypeOptimization(MethodDesc method)
+        {
+            return false;
+        }
     }
 }

src/coreclr/tools/aot/Mono.Linker.Tests/TestCasesRunner/TestInfraMultiFileCompilationModuleGroup.cs

@@ -47,5 +47,13 @@ public override bool AllowInstanceMethodOptimization (MethodDesc method)
 			}
 			return false;
 		}
+
+		public override bool AllowVirtualMethodOnAbstractTypeOptimization (MethodDesc method)
+		{
+			// Not really safe to do this since we need to assume IgnoreAccessChecks
+			// and we wouldn't know all derived types when compiling methods on the type
+			// that introduces this method.
+			return false;
+		}
 	}
 }

src/tests/nativeaot/SmokeTests/TrimmingBehaviors/DeadCodeElimination.cs

@@ -116,9 +116,7 @@ public static void Run()
                 s_d.TrySomething();
             }
 
-            // This optimization got disabled, but if it ever gets re-enabled, this test
-            // will ensure we don't reintroduce the old bugs (this was a compiler crash).
-            //ThrowIfPresent(typeof(TestAbstractTypeNeverDerivedVirtualsOptimization), nameof(UnreferencedType1));
+            ThrowIfPresent(typeof(TestAbstractTypeNeverDerivedVirtualsOptimization), nameof(UnreferencedType1));
         }
     }
 
@@ -147,9 +145,7 @@ public static void Run()
             // and uses a virtual method implementation from Base.
             DoIt(null);
 
-            // This optimization got disabled, but if it ever gets re-enabled, this test
-            // will ensure we don't reintroduce the old bugs (this was a compiler crash).
-            //ThrowIfPresent(typeof(TestAbstractNeverDerivedWithDevirtualizedCall), nameof(UnreferencedType1));
+            ThrowIfPresent(typeof(TestAbstractNeverDerivedWithDevirtualizedCall), nameof(UnreferencedType1));
         }
     }
 
@@ -185,9 +181,7 @@ public static void Run()
 
             new Derived2().DoSomething();
 
-            // This optimization got disabled, but if it ever gets re-enabled, this test
-            // will ensure we don't reintroduce the old bugs (this was a compiler crash).
-            //ThrowIfPresent(typeof(TestAbstractDerivedByUnrelatedTypeWithDevirtualizedCall), nameof(UnreferencedType1));
+            ThrowIfPresent(typeof(TestAbstractDerivedByUnrelatedTypeWithDevirtualizedCall), nameof(UnreferencedType1));
         }
     }