From: Michal Strehovský Date: Thu, 2 Mar 2023 01:15:31 +0000 (+0900) Subject: Try skipping generation of empty method dictionaries (#82591) X-Git-Tag: accepted/tizen/unified/riscv/20231226.055536~3741 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=0b82c57827fe69f2da829da95556d3872638ab0f;p=platform%2Fupstream%2Fdotnet%2Fruntime.git Try skipping generation of empty method dictionaries (#82591) Say we're compiling `Foo<__Canon>.Method` for: ```csharp class Foo { static void Method() => GenericMethod>(); static void GenericMethod() { } } ``` In the method body, we're generating a call to `GenericMethod<__Canon>` with a generic dictionary that we looked up from `Foo`s dictionary. But as you can see, the dictionary is empty because `GenericMethod` doesn't do anything with it's T. RyuJIT might even inline it. The problem is that we computed how the dictionary will look like during scanning and we're forever stuck with instruction to generate a generic dictionary for every `Foo` instantiation. This is adding an optimization - if during scanning we find out that the dictionary layout of the target method is empty, we instruct RyuJIT to generate a null generic context pointer. Saves 1.5% on BasicMinimalApi. Cc @dotnet/ilc-contrib --- diff --git a/src/coreclr/inc/corinfo.h b/src/coreclr/inc/corinfo.h index 61cd763..28531e0 100644 --- a/src/coreclr/inc/corinfo.h +++ b/src/coreclr/inc/corinfo.h @@ -1246,6 +1246,7 @@ struct CORINFO_LOOKUP_KIND // #define CORINFO_MAXINDIRECTIONS 4 #define CORINFO_USEHELPER ((uint16_t) 0xffff) +#define CORINFO_USENULL ((uint16_t) 0xfffe) #define CORINFO_NO_SIZE_CHECK ((uint16_t) 0xffff) struct CORINFO_RUNTIME_LOOKUP @@ -1258,6 +1259,7 @@ struct CORINFO_RUNTIME_LOOKUP // Number of indirections to get there // CORINFO_USEHELPER = don't know how to get it, so use helper function at run-time instead + // CORINFO_USENULL = the context should be null because the callee doesn't actually use it // 0 = use the this pointer itself (e.g. token is C inside code in sealed class C) // or method desc itself (e.g. token is method void M::mymeth() inside code in M::mymeth) // Otherwise, follow each byte-offset stored in the "offsets[]" array (may be negative) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 6b79460..6a9bbe2 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -1743,7 +1743,9 @@ GenTree* Compiler::getRuntimeContextTree(CORINFO_RUNTIME_LOOKUP_KIND kind) 1. pLookup->indirections == CORINFO_USEHELPER : Call a helper passing it the instantiation-specific handle, and the tokens to lookup the handle. - 2. pLookup->indirections != CORINFO_USEHELPER : + 2. pLookup->indirections == CORINFO_USENULL : Pass null. Callee won't dereference + the context. + 3. pLookup->indirections != CORINFO_USEHELPER or CORINFO_USENULL : 2a. pLookup->testForNull == false : Dereference the instantiation-specific handle to get the handle. 2b. pLookup->testForNull == true : Dereference the instantiation-specific handle. @@ -1771,6 +1773,13 @@ GenTree* Compiler::impRuntimeLookupToTree(CORINFO_RESOLVED_TOKEN* pResolvedToken return gtNewRuntimeLookupHelperCallNode(pRuntimeLookup, ctxTree, compileTimeHandle); } +#ifdef FEATURE_READYTORUN + if (pRuntimeLookup->indirections == CORINFO_USENULL) + { + return gtNewIconNode(0, TYP_I_IMPL); + } +#endif + // Slot pointer GenTree* slotPtrTree = ctxTree; diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 554604e..71f709d 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -7054,8 +7054,8 @@ GenTree* Compiler::getRuntimeLookupTree(CORINFO_RESOLVED_TOKEN* pResolvedToken, // If pRuntimeLookup->indirections is equal to CORINFO_USEHELPER, it specifies that a run-time helper should be // used; otherwise, it specifies the number of indirections via pRuntimeLookup->offsets array. - if ((pRuntimeLookup->indirections == CORINFO_USEHELPER) || pRuntimeLookup->testForNull || - pRuntimeLookup->testForFixup) + if ((pRuntimeLookup->indirections == CORINFO_USEHELPER) || (pRuntimeLookup->indirections == CORINFO_USENULL) || + pRuntimeLookup->testForNull || pRuntimeLookup->testForFixup) { // If the first condition is true, runtime lookup tree is available only via the run-time helper function. // TODO-CQ If the second or third condition is true, we are always using the slow path since we can't diff --git a/src/coreclr/tools/Common/JitInterface/CorInfoTypes.cs b/src/coreclr/tools/Common/JitInterface/CorInfoTypes.cs index 0b01f0e..a3d9206 100644 --- a/src/coreclr/tools/Common/JitInterface/CorInfoTypes.cs +++ b/src/coreclr/tools/Common/JitInterface/CorInfoTypes.cs @@ -21,6 +21,7 @@ namespace Internal.JitInterface // This accounts for up to 2 indirections to get at a dictionary followed by a possible spill slot public const uint MAXINDIRECTIONS = 4; public const ushort USEHELPER = 0xffff; + public const ushort USENULL = 0xfffe; public const ushort CORINFO_NO_SIZE_CHECK = 0xffff; } diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Compilation.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Compilation.cs index b5b6d57..d52b6f2 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Compilation.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Compilation.cs @@ -396,8 +396,7 @@ namespace ILCompiler int pointerSize = _nodeFactory.Target.PointerSize; GenericLookupResult lookup = ReadyToRunGenericHelperNode.GetLookupSignature(_nodeFactory, lookupKind, targetOfLookup); - int dictionarySlot = dictionaryLayout.GetSlotForFixedEntry(lookup); - if (dictionarySlot != -1) + if (dictionaryLayout.TryGetSlotForEntry(lookup, out int dictionarySlot)) { int dictionaryOffset = dictionarySlot * pointerSize; @@ -414,6 +413,10 @@ namespace ILCompiler return GenericDictionaryLookup.CreateFixedLookup(contextSource, vtableOffset, dictionaryOffset, indirectLastOffset: indirectLastOffset); } } + else + { + return GenericDictionaryLookup.CreateNullLookup(contextSource); + } } } diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/DictionaryLayoutNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/DictionaryLayoutNode.cs index d0faaf5..c3b7828 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/DictionaryLayoutNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/DictionaryLayoutNode.cs @@ -77,19 +77,14 @@ namespace ILCompiler.DependencyAnalysis /// /// Get a slot index for a given entry. Slot indices are never expected to change once given out. /// - public abstract int GetSlotForEntry(GenericLookupResult entry); + public abstract bool TryGetSlotForEntry(GenericLookupResult entry, out int slot); - /// - /// Get the slot for an entry which is fixed already. Otherwise return -1 - /// - /// - /// - public virtual int GetSlotForFixedEntry(GenericLookupResult entry) + public abstract IEnumerable Entries { - return GetSlotForEntry(entry); + get; } - public abstract IEnumerable Entries + public abstract bool IsEmpty { get; } @@ -195,41 +190,39 @@ namespace ILCompiler.DependencyAnalysis public class PrecomputedDictionaryLayoutNode : DictionaryLayoutNode { private readonly GenericLookupResult[] _layout; + private readonly GenericLookupResult[] _discardedSlots; public override bool HasFixedSlots => true; - public PrecomputedDictionaryLayoutNode(TypeSystemEntity owningMethodOrType, IEnumerable layout) + public override bool IsEmpty => _layout.Length == 0; + + public PrecomputedDictionaryLayoutNode(TypeSystemEntity owningMethodOrType, GenericLookupResult[] layout, GenericLookupResult[] discardedSlots) : base(owningMethodOrType) { - ArrayBuilder l = default(ArrayBuilder); - foreach (var entry in layout) - l.Add(entry); - - _layout = l.ToArray(); + _layout = layout; + _discardedSlots = discardedSlots; } public override void EnsureEntry(GenericLookupResult entry) { - int index = Array.IndexOf(_layout, entry); - - if (index == -1) - { - // Using EnsureEntry to add a slot to a PrecomputedDictionaryLayoutNode is not supported - throw new NotSupportedException(); - } + throw new NotSupportedException(); } - public override int GetSlotForEntry(GenericLookupResult entry) + public override bool TryGetSlotForEntry(GenericLookupResult entry, out int slot) { - int index = Array.IndexOf(_layout, entry); + slot = Array.IndexOf(_layout, entry); + + // If this is a slot we should discard, respond false + if (slot < 0 && Array.IndexOf(_discardedSlots, entry) >= 0) + return false; // This entry wasn't precomputed. If this is an optimized build with scanner, it might suggest // the scanner didn't see the need for this. There is a discrepancy between scanning and compiling. // This is a fatal error to prevent bad codegen. - if (index < 0) + if (slot < 0) throw new InvalidOperationException($"{OwningMethodOrType}: {entry}"); - return index; + return true; } public override IEnumerable Entries @@ -284,19 +277,19 @@ namespace ILCompiler.DependencyAnalysis _layout = layout; } - public override int GetSlotForEntry(GenericLookupResult entry) + public override bool TryGetSlotForEntry(GenericLookupResult entry, out int slot) { if (_layout == null) ComputeLayout(); - int index = Array.IndexOf(_layout, entry); + slot = Array.IndexOf(_layout, entry); // We never called EnsureEntry on this during compilation? // This is a fatal error to prevent bad codegen. - if (index < 0) + if (slot < 0) throw new InvalidOperationException($"{OwningMethodOrType}: {entry}"); - return index; + return true; } public override IEnumerable Entries @@ -309,5 +302,16 @@ namespace ILCompiler.DependencyAnalysis return _layout; } } + + public override bool IsEmpty + { + get + { + if (_layout == null) + ComputeLayout(); + + return _layout.Length == 0; + } + } } } diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_ARM/ARMReadyToRunGenericHelperNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_ARM/ARMReadyToRunGenericHelperNode.cs index 87c3c69..65e4e9f 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_ARM/ARMReadyToRunGenericHelperNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_ARM/ARMReadyToRunGenericHelperNode.cs @@ -30,7 +30,11 @@ namespace ILCompiler.DependencyAnalysis if (!relocsOnly) { // The concrete slot won't be known until we're emitting data - don't ask for it in relocsOnly. - dictionarySlot = factory.GenericDictionaryLayout(_dictionaryOwner).GetSlotForEntry(lookup); + if (!factory.GenericDictionaryLayout(_dictionaryOwner).TryGetSlotForEntry(lookup, out dictionarySlot)) + { + encoder.EmitMOV(result, 0); + return; + } } // Load the generic dictionary cell diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_ARM64/ARM64ReadyToRunGenericHelperNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_ARM64/ARM64ReadyToRunGenericHelperNode.cs index d017da8..0513017 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_ARM64/ARM64ReadyToRunGenericHelperNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_ARM64/ARM64ReadyToRunGenericHelperNode.cs @@ -30,7 +30,11 @@ namespace ILCompiler.DependencyAnalysis if (!relocsOnly) { // The concrete slot won't be known until we're emitting data - don't ask for it in relocsOnly. - dictionarySlot = factory.GenericDictionaryLayout(_dictionaryOwner).GetSlotForEntry(lookup); + if (!factory.GenericDictionaryLayout(_dictionaryOwner).TryGetSlotForEntry(lookup, out dictionarySlot)) + { + encoder.EmitMOV(result, (ushort)0); + return; + } } // Load the generic dictionary cell diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_LoongArch64/LoongArch64ReadyToRunGenericHelperNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_LoongArch64/LoongArch64ReadyToRunGenericHelperNode.cs index 1b6f404..ee80999 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_LoongArch64/LoongArch64ReadyToRunGenericHelperNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_LoongArch64/LoongArch64ReadyToRunGenericHelperNode.cs @@ -30,7 +30,11 @@ namespace ILCompiler.DependencyAnalysis if (!relocsOnly) { // The concrete slot won't be known until we're emitting data - don't ask for it in relocsOnly. - dictionarySlot = factory.GenericDictionaryLayout(_dictionaryOwner).GetSlotForEntry(lookup); + if (!factory.GenericDictionaryLayout(_dictionaryOwner).TryGetSlotForEntry(lookup, out dictionarySlot)) + { + encoder.EmitMOV(result, (ushort)0); + return; + } } // Load the generic dictionary cell diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_X64/X64ReadyToRunGenericHelperNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_X64/X64ReadyToRunGenericHelperNode.cs index 447393d..3b09eb2 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_X64/X64ReadyToRunGenericHelperNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_X64/X64ReadyToRunGenericHelperNode.cs @@ -30,7 +30,11 @@ namespace ILCompiler.DependencyAnalysis if (!relocsOnly) { // The concrete slot won't be known until we're emitting data - don't ask for it in relocsOnly. - dictionarySlot = factory.GenericDictionaryLayout(_dictionaryOwner).GetSlotForEntry(lookup); + if (!factory.GenericDictionaryLayout(_dictionaryOwner).TryGetSlotForEntry(lookup, out dictionarySlot)) + { + encoder.EmitZeroReg(result); + return; + } } // Load the generic dictionary cell diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/GenericDictionaryLookup.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/GenericDictionaryLookup.cs index a6e088f..7c65a39 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/GenericDictionaryLookup.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/GenericDictionaryLookup.cs @@ -16,6 +16,7 @@ namespace ILCompiler public struct GenericDictionaryLookup { private const short UseHelperOffset = -1; + private const short UseNullOffset = -2; private readonly object _helperObject; @@ -66,6 +67,14 @@ namespace ILCompiler } } + public bool UseNull + { + get + { + return _offset1 == UseNullOffset; + } + } + /// /// Gets the number of indirections to follow. Only valid if is false. /// @@ -73,7 +82,7 @@ namespace ILCompiler { get { - Debug.Assert(!UseHelper); + Debug.Assert(!UseHelper && !UseNull); return ContextSource == GenericContextSource.MethodParameter ? 1 : 2; } } @@ -82,7 +91,7 @@ namespace ILCompiler { get { - Debug.Assert(!UseHelper); + Debug.Assert(!UseHelper && !UseNull); Debug.Assert(index < NumberOfIndirections); switch (index) { @@ -101,7 +110,7 @@ namespace ILCompiler { get { - Debug.Assert(!UseHelper); + Debug.Assert(!UseHelper && !UseNull); return _indirectLastOffset; } } @@ -125,6 +134,11 @@ namespace ILCompiler { return new GenericDictionaryLookup(contextSource, UseHelperOffset, checked((short)helperId), helperObject, indirectLastOffset: false); } + + public static GenericDictionaryLookup CreateNullLookup(GenericContextSource contextSource) + { + return new GenericDictionaryLookup(contextSource, UseNullOffset, 0, null, false); + } } /// diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ILScanner.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ILScanner.cs index 716b61e..fa532f0 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ILScanner.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ILScanner.cs @@ -296,7 +296,7 @@ namespace ILCompiler private sealed class ScannedDictionaryLayoutProvider : DictionaryLayoutProvider { - private Dictionary> _layouts = new Dictionary>(); + private Dictionary _layouts = new(); private HashSet _entitiesWithForcedLazyLookups = new HashSet(); public ScannedDictionaryLayoutProvider(NodeFactory factory, ImmutableArray> markedNodes) @@ -306,7 +306,8 @@ namespace ILCompiler if (node is DictionaryLayoutNode layoutNode) { TypeSystemEntity owningMethodOrType = layoutNode.OwningMethodOrType; - _layouts.Add(owningMethodOrType, layoutNode.Entries); + GenericLookupResult[] layout = OptimizeSlots(factory, layoutNode.Entries, out GenericLookupResult[] discarded); + _layouts.Add(owningMethodOrType, (layout, discarded)); } else if (node is ReadyToRunGenericHelperNode genericLookup && genericLookup.HandlesInvalidEntries(factory)) @@ -320,9 +321,41 @@ namespace ILCompiler } } + private static GenericLookupResult[] OptimizeSlots(NodeFactory factory, IEnumerable slots, out GenericLookupResult[] discarded) + { + ArrayBuilder slotBuilder = default; + ArrayBuilder discardedBuilder = default; + + // We go over all slots in the layout, looking for references to method dictionaries + // that are going to be empty. + // Set those slots aside so that we can avoid generating the references to such dictionaries. + // We do this for methods only because method dictionaries have a high overhead (they + // get prefixed with a pointer-padded 32-bit hashcode and might end up in various + // summary tables as well). + + foreach (GenericLookupResult lookupResult in slots) + { + if (lookupResult is MethodDictionaryGenericLookupResult methodDictLookup) + { + MethodDesc targetMethod = methodDictLookup.Method.GetCanonMethodTarget(CanonicalFormKind.Specific); + DictionaryLayoutNode targetLayout = factory.GenericDictionaryLayout(targetMethod); + if (targetLayout.IsEmpty) + { + discardedBuilder.Add(lookupResult); + continue; + } + } + + slotBuilder.Add(lookupResult); + } + + discarded = discardedBuilder.ToArray(); + return slotBuilder.ToArray(); + } + private PrecomputedDictionaryLayoutNode GetPrecomputedLayout(TypeSystemEntity methodOrType) { - if (!_layouts.TryGetValue(methodOrType, out IEnumerable layout)) + if (!_layouts.TryGetValue(methodOrType, out var layout)) { // If we couldn't find the dictionary layout information for this, it's because the scanner // didn't correctly predict what will be needed. @@ -334,7 +367,7 @@ namespace ILCompiler Debug.Assert(false); throw new ScannerFailedException($"A dictionary layout was not computed by the IL scanner."); } - return new PrecomputedDictionaryLayoutNode(methodOrType, layout); + return new PrecomputedDictionaryLayoutNode(methodOrType, layout.Slots, layout.DiscardedSlots); } public override DictionaryLayoutNode GetLayout(TypeSystemEntity methodOrType) diff --git a/src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.cs b/src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.cs index 57f0fa1..c816c6f 100644 --- a/src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.cs +++ b/src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.cs @@ -261,6 +261,10 @@ namespace Internal.JitInterface lookup.lookupKind.runtimeLookupFlags = (ushort)genericLookup.HelperId; lookup.lookupKind.runtimeLookupArgs = (void*)ObjectToHandle(genericLookup.HelperObject); } + else if (genericLookup.UseNull) + { + lookup.runtimeLookup.indirections = CORINFO.USENULL; + } else { if (genericLookup.ContextSource == GenericContextSource.MethodParameter)