From 6a3ad50abcde2e22016662033d4db81106d2509c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Wed, 17 Jan 2024 09:16:45 +0100 Subject: [PATCH 1/2] Dead code elimination for `if (typeof(T).IsValueType)` @stephentoub found out that for following code: ```csharp using System.Buffers; Foo(); static T[] Foo() { if (typeof(T).IsValueType) { return ArrayPool.Shared.Rent(42); } return null!; } class Bar {} ``` We end up generating `ArrayPool`s of `Bar` even though it's obviously never reachable. The problem is architectural: * We run a whole program analysis phase that tries to figure out things like generic dictionary layouts so that later, in code generation phase, we can inline offsets into generic dictionaries into codegen. * For the above code, whole program analysis decides that the dictionary layout of `Foo<__Canon>` needs a slot for `ArrayPool`. * Codegen then optimizes out the `IsValueType` branch because `__Canon` is never a valuetype. But we already allocated the dictionary slot and will fill it out, even though it ends up unused due to the optimization. We're going to run into issues like this until #83021 is addressed. Whole program analysis cannot currently assume a certain optimization happens because we don't know whether RyuJIT will do it. The only way we can "optimize" during whole program analysis is if we rewrite IL and give RyuJIT no saying in whether to do an optimization or not. Rewriting the IL is not great because it e.g. causes PGO data to not match. I don't like doing it, but there's nothing else we can do. This change extends dead block elimination to understand `typeof(X).IsValueType`. If we recognize a branch is reachable under this condition, we evaluate whether this is true or false and replace the basic block with nops. --- .../Compiler/FeatureSwitchManager.cs | 84 ++++++++++++++----- .../TrimmingBehaviors/DeadCodeElimination.cs | 26 ++++++ 2 files changed, 88 insertions(+), 22 deletions(-) diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/FeatureSwitchManager.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/FeatureSwitchManager.cs index 2b2617be0a362e..c472ab306d64ff 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/FeatureSwitchManager.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/FeatureSwitchManager.cs @@ -591,6 +591,12 @@ private bool TryGetConstantArgument(MethodIL methodIL, byte[] body, OpcodeFlags[ { return true; } + else if (method.IsIntrinsic && method.Name is "get_IsValueType" + && method.OwningType is MetadataType { Name: "Type", Namespace: "System" } + && TryExpandTypeIsValueType(methodIL, body, flags, currentOffset, out constant)) + { + return true; + } else { constant = 0; @@ -745,6 +751,40 @@ private string GetResourceStringForAccessor(EcmaMethod method) return null; } + private static bool TryExpandTypeIsValueType(MethodIL methodIL, byte[] body, OpcodeFlags[] flags, int offset, out int constant) + { + // We expect to see a sequence: + // ldtoken Foo + // call GetTypeFromHandle + // -> offset points here + constant = 0; + const int SequenceLength = 10; + if (offset < SequenceLength) + return false; + + if ((flags[offset - SequenceLength] & OpcodeFlags.InstructionStart) == 0) + return false; + + ILReader reader = new ILReader(body, offset - SequenceLength); + + TypeDesc type = ReadLdToken(ref reader, methodIL, flags); + if (type == null) + return false; + + if (!ReadGetTypeFromHandle(ref reader, methodIL, flags)) + return false; + + // Dataflow runs on top of uninstantiated IL and we can't answer some questions there. + // Unfortunately this means dataflow will still see code that the rest of the system + // might have optimized away. It should not be a problem in practice. + if (type.IsSignatureVariable) + return false; + + constant = type.IsValueType ? 1 : 0; + + return true; + } + private static bool TryExpandTypeEquality(MethodIL methodIL, byte[] body, OpcodeFlags[] flags, int offset, string op, out int constant) { // We expect to see a sequence: @@ -793,37 +833,37 @@ private static bool TryExpandTypeEquality(MethodIL methodIL, byte[] body, Opcode constant ^= 1; return true; + } - static TypeDesc ReadLdToken(ref ILReader reader, MethodIL methodIL, OpcodeFlags[] flags) - { - ILOpcode opcode = reader.ReadILOpcode(); - if (opcode != ILOpcode.ldtoken) - return null; + private static TypeDesc ReadLdToken(ref ILReader reader, MethodIL methodIL, OpcodeFlags[] flags) + { + ILOpcode opcode = reader.ReadILOpcode(); + if (opcode != ILOpcode.ldtoken) + return null; - TypeDesc t = (TypeDesc)methodIL.GetObject(reader.ReadILToken()); + TypeDesc t = (TypeDesc)methodIL.GetObject(reader.ReadILToken()); - if ((flags[reader.Offset] & OpcodeFlags.BasicBlockStart) != 0) - return null; + if ((flags[reader.Offset] & OpcodeFlags.BasicBlockStart) != 0) + return null; - return t; - } + return t; + } - static bool ReadGetTypeFromHandle(ref ILReader reader, MethodIL methodIL, OpcodeFlags[] flags) - { - ILOpcode opcode = reader.ReadILOpcode(); - if (opcode != ILOpcode.call) - return false; + private static bool ReadGetTypeFromHandle(ref ILReader reader, MethodIL methodIL, OpcodeFlags[] flags) + { + ILOpcode opcode = reader.ReadILOpcode(); + if (opcode != ILOpcode.call) + return false; - MethodDesc method = (MethodDesc)methodIL.GetObject(reader.ReadILToken()); + MethodDesc method = (MethodDesc)methodIL.GetObject(reader.ReadILToken()); - if (!method.IsIntrinsic || method.Name != "GetTypeFromHandle") - return false; + if (!method.IsIntrinsic || method.Name != "GetTypeFromHandle") + return false; - if ((flags[reader.Offset] & OpcodeFlags.BasicBlockStart) != 0) - return false; + if ((flags[reader.Offset] & OpcodeFlags.BasicBlockStart) != 0) + return false; - return true; - } + return true; } private sealed class SubstitutedMethodIL : MethodIL diff --git a/src/tests/nativeaot/SmokeTests/TrimmingBehaviors/DeadCodeElimination.cs b/src/tests/nativeaot/SmokeTests/TrimmingBehaviors/DeadCodeElimination.cs index 858ecbbbe48040..21cc8c1fb63c56 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(); + TestTypeIsValueType.Run(); TestBranchesInGenericCodeRemoval.Run(); TestUnmodifiableStaticFieldOptimization.Run(); TestUnmodifiableInstanceFieldOptimization.Run(); @@ -355,6 +356,31 @@ public static void Run() } } + class TestTypeIsValueType + { + class Never { } + + class Ever { } + + static void Generic() + { + if (typeof(T).IsValueType) + { + Activator.CreateInstance(typeof(Never)); + } + + Activator.CreateInstance(typeof(Ever)); + } + + public static void Run() + { + Generic(); + + ThrowIfPresent(typeof(TestTypeIsValueType), nameof(Never)); + ThrowIfNotPresent(typeof(TestTypeIsValueType), nameof(Ever)); + } + } + class TestBranchesInGenericCodeRemoval { class ClassWithUnusedVirtual From 9ad404734368c674f7eb9de51fd1bf70c0c28c47 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Wed, 17 Jan 2024 22:39:58 +0100 Subject: [PATCH 2/2] fb --- .../aot/ILCompiler.Compiler/Compiler/FeatureSwitchManager.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/FeatureSwitchManager.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/FeatureSwitchManager.cs index c472ab306d64ff..beeb1ed11f6263 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/FeatureSwitchManager.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/FeatureSwitchManager.cs @@ -592,7 +592,8 @@ private bool TryGetConstantArgument(MethodIL methodIL, byte[] body, OpcodeFlags[ return true; } else if (method.IsIntrinsic && method.Name is "get_IsValueType" - && method.OwningType is MetadataType { Name: "Type", Namespace: "System" } + && method.OwningType is MetadataType mdt + && mdt.Name == "Type" && mdt.Namespace == "System" && mdt.Module == mdt.Context.SystemModule && TryExpandTypeIsValueType(methodIL, body, flags, currentOffset, out constant)) { return true;