From 9c9709779a932957446167b330c108fae6cd0369 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Thu, 2 Nov 2023 10:07:54 +0100 Subject: [PATCH 1/3] Restore EETypeNode to the state before we deleted reflection blocking Contributes to #91704. When we deleted reflection blocking in #85810 we had to update `EETypeNode`/`ConstructedEETypeNode` to ensure any `MethodTable` also has metadata (constructed or not). This was needed because of how reflection was structured - we couldn't create a `RuntimeType` without knowing about the metadata. After the refactor in #93440, metadata is no longer a prerequisite to constructing a `RuntimeType`. The compiler can go back to optimizing away the metadata. This affects things like `typeof(Foo) == bar.GetType()`. No metadata is needed for this (and we do optimized the `Foo` MethodTable to unconstructed one) but we still had to generate metadata. Besides the rollback of EEType to the previous shape, this also has a bugfix for #91988 that was found later - interface types used in cast/dispatch should be considered constructed. I'm seeing 0.1 - 0.7% size saving. --- .../ILCompiler.Compiler/Compiler/Compilation.cs | 11 +++++++++-- .../DependencyAnalysis/ConstructedEETypeNode.cs | 6 ++++++ .../Compiler/DependencyAnalysis/EETypeNode.cs | 14 +++++++++----- .../InterfaceDispatchCellNode.cs | 10 ++++++++-- .../TrimmingBehaviors/DeadCodeElimination.cs | 4 ++-- 5 files changed, 34 insertions(+), 11 deletions(-) diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Compilation.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Compilation.cs index 1fda33a405bb77..1e350711742454 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Compilation.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Compilation.cs @@ -301,9 +301,16 @@ public ISymbolNode ComputeConstantLookup(ReadyToRunHelperId lookupKind, object t case ReadyToRunHelperId.TypeHandleForCasting: { var type = (TypeDesc)targetOfLookup; + + // We counter-intuitively ask for a constructed type symbol. This is needed due to IDynamicInterfaceCastable. + // If this cast happens with an object that implements IDynamicIntefaceCastable, user code will + // see a RuntimeTypeHandle representing this interface. + if (type.IsInterface) + return NodeFactory.MaximallyConstructableType(type); + if (type.IsNullable) - targetOfLookup = type.Instantiation[0]; - return NecessaryTypeSymbolIfPossible((TypeDesc)targetOfLookup); + type = type.Instantiation[0]; + return NecessaryTypeSymbolIfPossible(type); } case ReadyToRunHelperId.MethodDictionary: return NodeFactory.MethodGenericDictionary((MethodDesc)targetOfLookup); diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ConstructedEETypeNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ConstructedEETypeNode.cs index 0d3f97754c8d8c..4331b0dd5416ac 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ConstructedEETypeNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ConstructedEETypeNode.cs @@ -31,6 +31,9 @@ protected override DependencyList ComputeNonRelocationBasedDependencies(NodeFact // relocs to nodes we emit. dependencyList.Add(factory.NecessaryTypeSymbol(_type), "NecessaryType for constructed type"); + if (_type is MetadataType mdType) + ModuleUseBasedDependencyAlgorithm.AddDependenciesDueToModuleUse(ref dependencyList, factory, mdType.Module); + DefType closestDefType = _type.GetClosestDefType(); if (_type.IsArray) @@ -66,6 +69,9 @@ protected override DependencyList ComputeNonRelocationBasedDependencies(NodeFact } } + // Ask the metadata manager if we have any dependencies due to the presence of the EEType. + factory.MetadataManager.GetDependenciesDueToEETypePresence(ref dependencyList, factory, _type); + factory.InteropStubManager.AddInterestingInteropConstructedTypeDependencies(ref dependencyList, factory, _type); return dependencyList; diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/EETypeNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/EETypeNode.cs index beb16cf020085c..f0e68ea0077bf3 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/EETypeNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/EETypeNode.cs @@ -633,12 +633,16 @@ protected override DependencyList ComputeNonRelocationBasedDependencies(NodeFact } } - // Ask the metadata manager - // if we have any dependencies due to presence of the EEType. - factory.MetadataManager.GetDependenciesDueToEETypePresence(ref dependencies, factory, _type); + if (!ConstructedEETypeNode.CreationAllowed(_type)) + { + // If necessary MethodTable is the highest load level for this type, ask the metadata manager + // if we have any dependencies due to presence of the EEType. + factory.MetadataManager.GetDependenciesDueToEETypePresence(ref dependencies, factory, _type); - if (_type is MetadataType mdType) - ModuleUseBasedDependencyAlgorithm.AddDependenciesDueToModuleUse(ref dependencies, factory, mdType.Module); + // If necessary MethodTable is the highest load level, consider this a module use + if (_type is MetadataType mdType) + ModuleUseBasedDependencyAlgorithm.AddDependenciesDueToModuleUse(ref dependencies, factory, mdType.Module); + } if (_type.IsFunctionPointer) FunctionPointerMapNode.GetHashtableDependencies(ref dependencies, factory, (FunctionPointerType)_type); diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/InterfaceDispatchCellNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/InterfaceDispatchCellNode.cs index d0f5ce91d6c3fb..aa3883a2db509b 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/InterfaceDispatchCellNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/InterfaceDispatchCellNode.cs @@ -71,7 +71,10 @@ public override IEnumerable GetStaticDependencies(NodeFacto result.Add(factory.ExternSymbol("RhpInitialDynamicInterfaceDispatch"), "Initial interface dispatch stub"); } - result.Add(factory.NecessaryTypeSymbol(_targetMethod.OwningType), "Interface type"); + // We counter-intuitively ask for a constructed type symbol. This is needed due to IDynamicInterfaceCastable. + // If this dispatch cell is ever used with an object that implements IDynamicIntefaceCastable, user code will + // see a RuntimeTypeHandle representing this interface. + result.Add(factory.ConstructedTypeSymbol(_targetMethod.OwningType), "Interface type"); return result; } @@ -88,7 +91,10 @@ public override void EncodeData(ref ObjectDataBuilder objData, NodeFactory facto objData.EmitPointerReloc(factory.ExternSymbol("RhpInitialDynamicInterfaceDispatch")); } - IEETypeNode interfaceType = factory.NecessaryTypeSymbol(_targetMethod.OwningType); + // We counter-intuitively ask for a constructed type symbol. This is needed due to IDynamicInterfaceCastable. + // If this dispatch cell is ever used with an object that implements IDynamicIntefaceCastable, user code will + // see a RuntimeTypeHandle representing this interface. + IEETypeNode interfaceType = factory.ConstructedTypeSymbol(_targetMethod.OwningType); if (factory.Target.SupportsRelativePointers) { if (interfaceType.RepresentsIndirectionCell) diff --git a/src/tests/nativeaot/SmokeTests/TrimmingBehaviors/DeadCodeElimination.cs b/src/tests/nativeaot/SmokeTests/TrimmingBehaviors/DeadCodeElimination.cs index c0c87c847995ea..dbadaf5cd78de7 100644 --- a/src/tests/nativeaot/SmokeTests/TrimmingBehaviors/DeadCodeElimination.cs +++ b/src/tests/nativeaot/SmokeTests/TrimmingBehaviors/DeadCodeElimination.cs @@ -322,7 +322,7 @@ public static void Run() Console.WriteLine(s_type == typeof(Never)); #if !DEBUG - ThrowIfPresentWithUsableMethodTable(typeof(TestTypeEquals), nameof(Never)); + ThrowIfPresent(typeof(TestTypeEquals), nameof(Never)); #endif } } @@ -371,7 +371,7 @@ public static void Run() // We only expect to be able to get rid of it when optimizing #if !DEBUG - ThrowIfPresentWithUsableMethodTable(typeof(TestBranchesInGenericCodeRemoval), nameof(Unused)); + ThrowIfPresent(typeof(TestBranchesInGenericCodeRemoval), nameof(Unused)); #endif ThrowIfNotPresent(typeof(TestBranchesInGenericCodeRemoval), nameof(Used)); From 3d9ea1c0fadf619a27741fa725fb7e529c20f91b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Thu, 2 Nov 2023 13:56:30 +0100 Subject: [PATCH 2/3] Tweak --- .../ILCompiler.Compiler/Compiler/UsageBasedMetadataManager.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/UsageBasedMetadataManager.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/UsageBasedMetadataManager.cs index bd3e1069881361..fc7c381242c735 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/UsageBasedMetadataManager.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/UsageBasedMetadataManager.cs @@ -660,7 +660,7 @@ public override IEnumerable GetCompilationModulesWithMetadata() private IEnumerable GetTypesWithRuntimeMapping() { // All constructed types that are not blocked get runtime mapping - foreach (var constructedType in GetTypesWithEETypes()) + foreach (var constructedType in GetTypesWithConstructedEETypes()) { if (!IsReflectionBlocked(constructedType)) yield return constructedType; From acd2e6be11c4a70f6dcf73cbccf3fc2c1253721a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Mon, 6 Nov 2023 08:25:25 +0100 Subject: [PATCH 3/3] Rewind all the way back to CoreRT https://github.com/dotnet/corert/blame/c6af4cfc8b625851b91823d9be746c4f7abdc667/src/ILCompiler.Compiler/src/Compiler/UsageBasedMetadataManager.cs#L406-L424 --- .../Compiler/UsageBasedMetadataManager.cs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/UsageBasedMetadataManager.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/UsageBasedMetadataManager.cs index fc7c381242c735..7715ce99a73730 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/UsageBasedMetadataManager.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/UsageBasedMetadataManager.cs @@ -665,6 +665,15 @@ private IEnumerable GetTypesWithRuntimeMapping() if (!IsReflectionBlocked(constructedType)) yield return constructedType; } + + // All necessary types for which this is the highest load level that are not blocked + // get runtime mapping. + foreach (var necessaryType in GetTypesWithEETypes()) + { + if (!ConstructedEETypeNode.CreationAllowed(necessaryType) && + !IsReflectionBlocked(necessaryType)) + yield return necessaryType; + } } public override void GetDependenciesDueToAccess(ref DependencyList dependencies, NodeFactory factory, MethodIL methodIL, FieldDesc writtenField)