From 9c7ee976fd771c183e98cf629e3776bba4e45ccc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Thu, 18 Jul 2024 16:51:38 +0200 Subject: [PATCH] Avoid scanning typeof checks when building whole program view (#103883) Before this PR, we were somewhat able to eliminate dead typeof checks such as: ```csharp if (someType == typeof(Foo) { ExpensiveMethod(); } ``` This work was done in https://github.com/dotnet/runtime/pull/102248. However, the optimization only happened during codegen. This meant that when building the whole program view, we'd still look at `ExpensiveMethod` and whatever damage this caused to the whole program view was permanent. With this PR, the scanner now becomes aware of the optimization we do during codegen and tries to defer injecting dependencies until we will need them. With this change, we detect the conditional branch, and generate whatever dependencies from the basic block as conditional. That way scanning can fully skip scanning `ExpensiveMethod` and the subsequent optimization will ensure the missed scanning will not cause issues at codegen time. --- .../ShadowConcreteMethodNode.cs | 27 +++- .../tools/Common/TypeSystem/IL/ILImporter.cs | 9 +- .../DependencyAnalysis/ScannedMethodNode.cs | 18 ++- .../IL/ILImporter.Scanner.cs | 115 ++++++++++++++++-- .../SmokeTests/HardwareIntrinsics/Program.cs | 4 +- .../TrimmingBehaviors/DeadCodeElimination.cs | 37 ++++++ 6 files changed, 184 insertions(+), 26 deletions(-) diff --git a/src/coreclr/tools/Common/Compiler/DependencyAnalysis/ShadowConcreteMethodNode.cs b/src/coreclr/tools/Common/Compiler/DependencyAnalysis/ShadowConcreteMethodNode.cs index 140a97f70924e..4480f9d394103 100644 --- a/src/coreclr/tools/Common/Compiler/DependencyAnalysis/ShadowConcreteMethodNode.cs +++ b/src/coreclr/tools/Common/Compiler/DependencyAnalysis/ShadowConcreteMethodNode.cs @@ -85,14 +85,37 @@ public override IEnumerable GetStaticDependencies(NodeFacto return dependencies; } + public sealed override IEnumerable GetConditionalStaticDependencies(NodeFactory factory) + { + // Instantiate the runtime determined dependencies of the canonical method body + // with the concrete instantiation of the method to get concrete dependencies. + Instantiation typeInst = Method.OwningType.Instantiation; + Instantiation methodInst = Method.Instantiation; + IEnumerable staticDependencies = CanonicalMethodNode.GetConditionalStaticDependencies(factory); + + if (staticDependencies != null) + { + foreach (CombinedDependencyListEntry canonDep in staticDependencies) + { + Debug.Assert(canonDep.OtherReasonNode is not INodeWithRuntimeDeterminedDependencies); + + var node = canonDep.Node; + if (node is INodeWithRuntimeDeterminedDependencies runtimeDeterminedNode) + { + foreach (var nodeInner in runtimeDeterminedNode.InstantiateDependencies(factory, typeInst, methodInst)) + yield return new CombinedDependencyListEntry(nodeInner.Node, canonDep.OtherReasonNode, nodeInner.Reason); + } + } + } + } + protected override string GetName(NodeFactory factory) => $"{Method} backed by {CanonicalMethodNode.GetMangledName(factory.NameMangler)}"; - public sealed override bool HasConditionalStaticDependencies => false; + public sealed override bool HasConditionalStaticDependencies => CanonicalMethodNode.HasConditionalStaticDependencies; public sealed override bool HasDynamicDependencies => false; public sealed override bool InterestingForDynamicDependencyAnalysis => false; public sealed override IEnumerable SearchDynamicDependencies(List> markedNodes, int firstNode, NodeFactory factory) => null; - public sealed override IEnumerable GetConditionalStaticDependencies(NodeFactory factory) => null; int ISortableNode.ClassCode => -1440570971; diff --git a/src/coreclr/tools/Common/TypeSystem/IL/ILImporter.cs b/src/coreclr/tools/Common/TypeSystem/IL/ILImporter.cs index 26bc612b8262d..afc10fce167ba 100644 --- a/src/coreclr/tools/Common/TypeSystem/IL/ILImporter.cs +++ b/src/coreclr/tools/Common/TypeSystem/IL/ILImporter.cs @@ -301,12 +301,17 @@ private void ImportBasicBlocks() } private void MarkBasicBlock(BasicBlock basicBlock) + { + MarkBasicBlock(basicBlock, ref _pendingBasicBlocks); + } + + private static void MarkBasicBlock(BasicBlock basicBlock, ref BasicBlock list) { if (basicBlock.State == BasicBlock.ImportState.Unmarked) { // Link - basicBlock.Next = _pendingBasicBlocks; - _pendingBasicBlocks = basicBlock; + basicBlock.Next = list; + list = basicBlock; basicBlock.State = BasicBlock.ImportState.IsPending; } diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ScannedMethodNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ScannedMethodNode.cs index c72491f130728..b4a443b4281b1 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ScannedMethodNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ScannedMethodNode.cs @@ -22,6 +22,7 @@ public class ScannedMethodNode : DependencyNodeCore, IMethodBodyNod { private readonly MethodDesc _method; private DependencyList _dependencies; + private CombinedDependencyList _conditionalDependencies; // If we failed to scan the method, the dependencies reported by the node will // be for a throwing method body. This field will store the underlying cause of the failure. @@ -42,11 +43,14 @@ public ScannedMethodNode(MethodDesc method) public bool RepresentsIndirectionCell => false; + public override bool HasConditionalStaticDependencies => _conditionalDependencies != null; + public override bool StaticDependenciesAreComputed => _dependencies != null; - public void InitializeDependencies(NodeFactory factory, IEnumerable dependencies, TypeSystemException scanningException = null) + public void InitializeDependencies(NodeFactory factory, (DependencyList, CombinedDependencyList) dependencies, TypeSystemException scanningException = null) { - _dependencies = new DependencyList(dependencies); + _dependencies = dependencies.Item1; + _conditionalDependencies = dependencies.Item2; if (factory.TypeSystemContext.IsSpecialUnboxingThunk(_method)) { @@ -72,19 +76,13 @@ public override IEnumerable GetStaticDependencies(NodeFacto return _dependencies; } - protected override string GetName(NodeFactory factory) => this.GetMangledName(factory.NameMangler); + public override IEnumerable GetConditionalStaticDependencies(NodeFactory factory) => _conditionalDependencies; - public override IEnumerable GetConditionalStaticDependencies(NodeFactory factory) - { - CombinedDependencyList dependencies = null; - CodeBasedDependencyAlgorithm.AddConditionalDependenciesDueToMethodCodePresence(ref dependencies, factory, _method); - return dependencies ?? (IEnumerable)Array.Empty(); - } + protected override string GetName(NodeFactory factory) => this.GetMangledName(factory.NameMangler); public override IEnumerable SearchDynamicDependencies(List> markedNodes, int firstNode, NodeFactory factory) => null; public override bool InterestingForDynamicDependencyAnalysis => _method.HasInstantiation || _method.OwningType.HasInstantiation; public override bool HasDynamicDependencies => false; - public override bool HasConditionalStaticDependencies => CodeBasedDependencyAlgorithm.HasConditionalDependenciesDueToMethodCodePresence(_method); int ISortableNode.ClassCode => -1381809560; diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/IL/ILImporter.Scanner.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/IL/ILImporter.Scanner.cs index 9952aedd18d40..81c35b2f72cbf 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/IL/ILImporter.Scanner.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/IL/ILImporter.Scanner.cs @@ -9,6 +9,8 @@ using Debug = System.Diagnostics.Debug; using DependencyList = ILCompiler.DependencyAnalysisFramework.DependencyNodeCore.DependencyList; +using CombinedDependencyList = System.Collections.Generic.List.CombinedDependencyListEntry>; +using DependencyListEntry = ILCompiler.DependencyAnalysisFramework.DependencyNodeCore.DependencyListEntry; #pragma warning disable IDE0060 @@ -28,7 +30,7 @@ internal partial class ILImporter private readonly MethodDesc _canonMethod; - private DependencyList _dependencies = new DependencyList(); + private DependencyList _unconditionalDependencies = new DependencyList(); private readonly byte[] _ilBytes; @@ -51,11 +53,17 @@ public enum ImportState : byte public bool TryStart; public bool FilterStart; public bool HandlerStart; + + public object Condition; + public DependencyList Dependencies; } private bool _isReadOnly; private TypeDesc _constrained; + private DependencyList _dependencies; + private BasicBlock _lateBasicBlocks; + private sealed class ExceptionRegion { public ILExceptionRegion ILRegion; @@ -107,9 +115,11 @@ public ILImporter(ILScanner compilation, MethodDesc method, MethodIL methodIL = { _exceptionRegions[i] = new ExceptionRegion() { ILRegion = ilExceptionRegions[i] }; } + + _dependencies = _unconditionalDependencies; } - public DependencyList Import() + public (DependencyList, CombinedDependencyList) Import() { TypeDesc owningType = _canonMethod.OwningType; if (_compilation.HasLazyStaticConstructor(owningType)) @@ -172,9 +182,21 @@ public DependencyList Import() FindBasicBlocks(); ImportBasicBlocks(); - CodeBasedDependencyAlgorithm.AddDependenciesDueToMethodCodePresence(ref _dependencies, _factory, _canonMethod, _canonMethodIL); + CombinedDependencyList conditionalDependencies = null; + foreach (BasicBlock bb in _basicBlocks) + { + if (bb?.Condition == null) + continue; + + conditionalDependencies ??= new CombinedDependencyList(); + foreach (DependencyListEntry dep in bb.Dependencies) + conditionalDependencies.Add(new(dep.Node, bb.Condition, dep.Reason)); + } + + CodeBasedDependencyAlgorithm.AddDependenciesDueToMethodCodePresence(ref _unconditionalDependencies, _factory, _canonMethod, _canonMethodIL); + CodeBasedDependencyAlgorithm.AddConditionalDependenciesDueToMethodCodePresence(ref conditionalDependencies, _factory, _canonMethod); - return _dependencies; + return (_unconditionalDependencies, conditionalDependencies); } private ISymbolNode GetGenericLookupHelper(ReadyToRunHelperId helperId, object helperArgument) @@ -199,19 +221,29 @@ private ISymbolNode GetHelperEntrypoint(ReadyToRunHelper helper) } private static void MarkInstructionBoundary() { } - private static void EndImportingBasicBlock(BasicBlock basicBlock) { } + + private void EndImportingBasicBlock(BasicBlock basicBlock) + { + if (_pendingBasicBlocks == null) + { + _pendingBasicBlocks = _lateBasicBlocks; + _lateBasicBlocks = null; + } + } private void StartImportingBasicBlock(BasicBlock basicBlock) { + _dependencies = basicBlock.Condition != null ? basicBlock.Dependencies : _unconditionalDependencies; + // Import all associated EH regions foreach (ExceptionRegion ehRegion in _exceptionRegions) { ILExceptionRegion region = ehRegion.ILRegion; if (region.TryOffset == basicBlock.StartOffset) { - MarkBasicBlock(_basicBlocks[region.HandlerOffset]); + ImportBasicBlockEdge(basicBlock, _basicBlocks[region.HandlerOffset]); if (region.Kind == ILExceptionRegionKind.Filter) - MarkBasicBlock(_basicBlocks[region.FilterOffset]); + ImportBasicBlockEdge(basicBlock, _basicBlocks[region.FilterOffset]); if (region.Kind == ILExceptionRegionKind.Catch) { @@ -789,10 +821,26 @@ private void ImportCalli(int token) private void ImportBranch(ILOpcode opcode, BasicBlock target, BasicBlock fallthrough) { + object condition = null; + + if (opcode == ILOpcode.brfalse + && _typeEqualityPatternAnalyzer.IsTypeEqualityBranch + && !_typeEqualityPatternAnalyzer.IsTwoTokens + && !_typeEqualityPatternAnalyzer.IsInequality) + { + TypeDesc typeEqualityCheckType = (TypeDesc)_canonMethodIL.GetObject(_typeEqualityPatternAnalyzer.Token1); + if (!typeEqualityCheckType.IsGenericDefinition + && ConstructedEETypeNode.CreationAllowed(typeEqualityCheckType) + && !typeEqualityCheckType.ConvertToCanonForm(CanonicalFormKind.Specific).IsCanonicalSubtype(CanonicalFormKind.Any)) + { + condition = _factory.MaximallyConstructableType(typeEqualityCheckType); + } + } + ImportFallthrough(target); if (fallthrough != null) - ImportFallthrough(fallthrough); + ImportFallthrough(fallthrough, condition); } private void ImportSwitchJump(int jmpBase, int[] jmpDelta, BasicBlock fallthrough) @@ -1278,9 +1326,56 @@ private void ImportConvert(WellKnownType wellKnownType, bool checkOverflow, bool } } - private void ImportFallthrough(BasicBlock next) + private void ImportBasicBlockEdge(BasicBlock source, BasicBlock next, object condition = null) + { + // We don't track multiple conditions; if the source basic block is only reachable under a condition, + // this condition will be used for the next basic block, irrespective if we could make it more narrow. + object effectiveCondition = source.Condition ?? condition; + + // Did we already look at this basic block? + if (next.State != BasicBlock.ImportState.Unmarked) + { + // If next is not conditioned, it stays not conditioned. + // If it was conditioned on something else, it needs to become unconditional. + // If the conditions match, it stays conditioned on the same thing. + if (next.Condition != null && next.Condition != effectiveCondition) + { + // Now we need to make `next` not conditioned. We move all of its dependencies to + // unconditional dependencies, and do this for all basic blocks that are reachable + // from it. + // TODO-SIZE: below doesn't do it for all basic blocks reachable from `next`, but + // for all basic blocks with the same conditon. This is a shortcut. It likely + // doesn't matter in practice. + object conditionToRemove = next.Condition; + foreach (BasicBlock bb in _basicBlocks) + { + if (bb?.Condition == conditionToRemove) + { + _unconditionalDependencies.AddRange(bb.Dependencies); + bb.Dependencies = null; + bb.Condition = null; + } + } + } + } + else + { + if (effectiveCondition != null) + { + next.Condition = effectiveCondition; + next.Dependencies = new DependencyList(); + MarkBasicBlock(next, ref _lateBasicBlocks); + } + else + { + MarkBasicBlock(next); + } + } + } + + private void ImportFallthrough(BasicBlock next, object condition = null) { - MarkBasicBlock(next); + ImportBasicBlockEdge(_currentBasicBlock, next, condition); } private int ReadILTokenAt(int ilOffset) diff --git a/src/tests/nativeaot/SmokeTests/HardwareIntrinsics/Program.cs b/src/tests/nativeaot/SmokeTests/HardwareIntrinsics/Program.cs index 28b678b5c2b22..43eed4a2b4eee 100644 --- a/src/tests/nativeaot/SmokeTests/HardwareIntrinsics/Program.cs +++ b/src/tests/nativeaot/SmokeTests/HardwareIntrinsics/Program.cs @@ -21,8 +21,8 @@ static int Main() Console.WriteLine("****************************************************"); long lowerBound, upperBound; - lowerBound = 1300 * 1024; // ~1.3 MB - upperBound = 1900 * 1024; // ~1.90 MB + lowerBound = 1200 * 1024; // ~1.2 MB + upperBound = 1600 * 1024; // ~1.6 MB if (fileSize < lowerBound || fileSize > upperBound) { diff --git a/src/tests/nativeaot/SmokeTests/TrimmingBehaviors/DeadCodeElimination.cs b/src/tests/nativeaot/SmokeTests/TrimmingBehaviors/DeadCodeElimination.cs index cae6055366873..ca5efa459dece 100644 --- a/src/tests/nativeaot/SmokeTests/TrimmingBehaviors/DeadCodeElimination.cs +++ b/src/tests/nativeaot/SmokeTests/TrimmingBehaviors/DeadCodeElimination.cs @@ -21,6 +21,7 @@ public static int Run() TestArrayElementTypeOperations.Run(); TestStaticVirtualMethodOptimizations.Run(); TestTypeEquals.Run(); + TestTypeEqualityDeadBranchScanRemoval.Run(); TestTypeIsEnum.Run(); TestTypeIsValueType.Run(); TestBranchesInGenericCodeRemoval.Run(); @@ -452,6 +453,42 @@ static void RunCheck(Type t) } } + class TestTypeEqualityDeadBranchScanRemoval + { + class NeverAllocated1 { } + class NeverAllocated2 { } + + class PossiblyAllocated1 { } + class PossiblyAllocated2 { } + + [MethodImpl(MethodImplOptions.NoInlining)] + static Type GetNeverObject() => null; + + static volatile Type s_sink; + + public static void Run() + { + if (GetNeverObject() == typeof(NeverAllocated1)) + { + Console.WriteLine(new NeverAllocated1().ToString()); + Console.WriteLine(new NeverAllocated2().ToString()); + } +#if !DEBUG + ThrowIfPresentWithUsableMethodTable(typeof(TestTypeEqualityDeadBranchScanRemoval), nameof(NeverAllocated1)); + ThrowIfPresent(typeof(TestTypeEqualityDeadBranchScanRemoval), nameof(NeverAllocated2)); +#endif + + if (GetNeverObject() == typeof(PossiblyAllocated1)) + { + Console.WriteLine(new PossiblyAllocated1().ToString()); + Console.WriteLine(new PossiblyAllocated2().ToString()); + } + if (Environment.GetEnvironmentVariable("SURETHING") != null) + s_sink = typeof(PossiblyAllocated1); + } + } + + class TestTypeIsEnum { class Never { }