diff options
| -rw-r--r-- | src/jit/flowgraph.cpp | 38 | ||||
| -rw-r--r-- | src/jit/gentree.h | 3 | ||||
| -rw-r--r-- | src/jit/inline.def | 1 | ||||
| -rw-r--r-- | src/jit/inline.h | 2 | ||||
| -rw-r--r-- | src/jit/inlinepolicy.cpp | 107 | ||||
| -rw-r--r-- | src/jit/inlinepolicy.h | 29 | ||||
| -rw-r--r-- | src/jit/jitconfigvalues.h | 1 | ||||
| -rwxr-xr-x | src/jit/morph.cpp | 25 | ||||
| -rw-r--r-- | tests/src/JIT/Performance/CodeQuality/Inlining/NoThrowInline.cs | 75 | ||||
| -rw-r--r-- | tests/src/JIT/Performance/CodeQuality/Inlining/NoThrowInline.csproj | 47 |
10 files changed, 318 insertions, 10 deletions
diff --git a/src/jit/flowgraph.cpp b/src/jit/flowgraph.cpp index 25bc4d7edc..4fc28abce8 100644 --- a/src/jit/flowgraph.cpp +++ b/src/jit/flowgraph.cpp @@ -5750,14 +5750,48 @@ void Compiler::fgFindBasicBlocks() if (compIsForInlining()) { + bool hasReturnBlocks = false; + bool hasMoreThanOneReturnBlock = false; + + for (BasicBlock* block = fgFirstBB; block != nullptr; block = block->bbNext) + { + if (block->bbJumpKind == BBJ_RETURN) + { + if (hasReturnBlocks) + { + hasMoreThanOneReturnBlock = true; + break; + } + + hasReturnBlocks = true; + } + } + + if (!hasReturnBlocks && !compInlineResult->UsesLegacyPolicy()) + { + // + // Mark the call node as "no return". The inliner might ignore CALLEE_DOES_NOT_RETURN and + // fail inline for a different reasons. In that case we still want to make the "no return" + // information available to the caller as it can impact caller's code quality. + // + + impInlineInfo->iciCall->gtCallMoreFlags |= GTF_CALL_M_DOES_NOT_RETURN; + } + + compInlineResult->NoteBool(InlineObservation::CALLEE_DOES_NOT_RETURN, !hasReturnBlocks); + + if (compInlineResult->IsFailure()) + { + return; + } + noway_assert(info.compXcptnsCount == 0); compHndBBtab = impInlineInfo->InlinerCompiler->compHndBBtab; compHndBBtabAllocCount = impInlineInfo->InlinerCompiler->compHndBBtabAllocCount; // we probably only use the table, not add to it. compHndBBtabCount = impInlineInfo->InlinerCompiler->compHndBBtabCount; info.compXcptnsCount = impInlineInfo->InlinerCompiler->info.compXcptnsCount; - if (info.compRetNativeType != TYP_VOID && - fgMoreThanOneReturnBlock()) + if (info.compRetNativeType != TYP_VOID && hasMoreThanOneReturnBlock) { // The lifetime of this var might expand multiple BBs. So it is a long lifetime compiler temp. lvaInlineeReturnSpillTemp = lvaGrabTemp(false DEBUGARG("Inline candidate multiple BBJ_RETURN spill temp")); diff --git a/src/jit/gentree.h b/src/jit/gentree.h index edfcfd6b6b..352c061b4e 100644 --- a/src/jit/gentree.h +++ b/src/jit/gentree.h @@ -2886,6 +2886,7 @@ struct GenTreeCall final : public GenTree // know when these flags are set. #define GTF_CALL_M_R2R_REL_INDIRECT 0x2000 // GT_CALL -- ready to run call is indirected through a relative address +#define GTF_CALL_M_DOES_NOT_RETURN 0x4000 // GT_CALL -- call does not return bool IsUnmanaged() const { return (gtFlags & GTF_CALL_UNMANAGED) != 0; } bool NeedsNullCheck() const { return (gtFlags & GTF_CALL_NULLCHECK) != 0; } @@ -3006,6 +3007,8 @@ struct GenTreeCall final : public GenTree bool IsVarargs() const { return (gtCallMoreFlags & GTF_CALL_M_VARARGS) != 0; } + bool IsNoReturn() const { return (gtCallMoreFlags & GTF_CALL_M_DOES_NOT_RETURN) != 0; } + unsigned short gtCallMoreFlags; // in addition to gtFlags unsigned char gtCallType :3; // value from the gtCallTypes enumeration diff --git a/src/jit/inline.def b/src/jit/inline.def index 69b617d4c3..9473bdaf00 100644 --- a/src/jit/inline.def +++ b/src/jit/inline.def @@ -75,6 +75,7 @@ INLINE_OBSERVATION(ARG_FEEDS_RANGE_CHECK, bool, "argument feeds range chec INLINE_OBSERVATION(BEGIN_OPCODE_SCAN, bool, "prepare to look at opcodes", INFORMATION, CALLEE) INLINE_OBSERVATION(BELOW_ALWAYS_INLINE_SIZE, bool, "below ALWAYS_INLINE size", INFORMATION, CALLEE) INLINE_OBSERVATION(CLASS_PROMOTABLE, bool, "promotable value class", INFORMATION, CALLEE) +INLINE_OBSERVATION(DOES_NOT_RETURN, bool, "does not return", INFORMATION, CALLEE) INLINE_OBSERVATION(END_OPCODE_SCAN, bool, "done looking at opcodes", INFORMATION, CALLEE) INLINE_OBSERVATION(HAS_SIMD, bool, "has SIMD arg, local, or ret", INFORMATION, CALLEE) INLINE_OBSERVATION(HAS_SWITCH, bool, "has switch", INFORMATION, CALLEE) diff --git a/src/jit/inline.h b/src/jit/inline.h index 8f91247d4b..ebfdb7337a 100644 --- a/src/jit/inline.h +++ b/src/jit/inline.h @@ -564,7 +564,7 @@ struct InlineInfo bool hasSIMDTypeArgLocalOrReturn; #endif // FEATURE_SIMD - GenTree * iciCall; // The GT_CALL node to be inlined. + GenTreeCall * iciCall; // The GT_CALL node to be inlined. GenTree * iciStmt; // The statement iciCall is in. BasicBlock * iciBlock; // The basic block iciStmt is in. }; diff --git a/src/jit/inlinepolicy.cpp b/src/jit/inlinepolicy.cpp index 28a9d21a55..a429d9e9c3 100644 --- a/src/jit/inlinepolicy.cpp +++ b/src/jit/inlinepolicy.cpp @@ -77,21 +77,24 @@ InlinePolicy* InlinePolicy::GetPolicy(Compiler* compiler, bool isPrejitRoot) #endif // defined(DEBUG) || defined(INLINE_DATA) - InlinePolicy* policy = nullptr; + // Optionally install the ModelPolicy. bool useModelPolicy = JitConfig.JitInlinePolicyModel() != 0; if (useModelPolicy) { - // Optionally install the ModelPolicy. - policy = new (compiler, CMK_Inlining) ModelPolicy(compiler, isPrejitRoot); + return new (compiler, CMK_Inlining) ModelPolicy(compiler, isPrejitRoot); } - else + + // Optionally fallback to the original legacy policy + bool useLegacyPolicy = JitConfig.JitInlinePolicyLegacy() != 0; + + if (useLegacyPolicy) { - // Use the legacy policy - policy = new (compiler, CMK_Inlining) LegacyPolicy(compiler, isPrejitRoot); + return new (compiler, CMK_Inlining) LegacyPolicy(compiler, isPrejitRoot); } - return policy; + // Use the enhanced legacy policy by default + return new (compiler, CMK_Inlining) EnhancedLegacyPolicy(compiler, isPrejitRoot); } //------------------------------------------------------------------------ @@ -849,6 +852,96 @@ int LegacyPolicy::CodeSizeEstimate() } } +//------------------------------------------------------------------------ +// NoteBool: handle a boolean observation with non-fatal impact +// +// Arguments: +// obs - the current obsevation +// value - the value of the observation + +void EnhancedLegacyPolicy::NoteBool(InlineObservation obs, bool value) +{ + switch (obs) + { + case InlineObservation::CALLEE_DOES_NOT_RETURN: + m_IsNoReturn = value; + m_IsNoReturnKnown = true; + break; + + default: + // Pass all other information to the legacy policy + LegacyPolicy::NoteBool(obs, value); + break; + } +} + +//------------------------------------------------------------------------ +// NoteInt: handle an observed integer value +// +// Arguments: +// obs - the current obsevation +// value - the value being observed + +void EnhancedLegacyPolicy::NoteInt(InlineObservation obs, int value) +{ + switch (obs) + { + case InlineObservation::CALLEE_NUMBER_OF_BASIC_BLOCKS: + { + assert(value != 0); + assert(m_IsNoReturnKnown); + + // + // Let's be conservative for now and reject inlining of "no return" methods only + // if the callee contains a single basic block. This covers most of the use cases + // (typical throw helpers simply do "throw new X();" and so they have a single block) + // without affecting more exotic cases (loops that do actual work for example) where + // failure to inline could negatively impact code quality. + // + + unsigned basicBlockCount = static_cast<unsigned>(value); + + if (m_IsNoReturn && (basicBlockCount == 1)) + { + SetNever(InlineObservation::CALLEE_DOES_NOT_RETURN); + } + else + { + LegacyPolicy::NoteInt(obs, value); + } + + break; + } + + default: + // Pass all other information to the legacy policy + LegacyPolicy::NoteInt(obs, value); + break; + } +} + +//------------------------------------------------------------------------ +// PropagateNeverToRuntime: determine if a never result should cause the +// method to be marked as un-inlinable. + +bool EnhancedLegacyPolicy::PropagateNeverToRuntime() const +{ + // + // Do not propagate the "no return" observation. If we do this then future inlining + // attempts will fail immediately without marking the call node as "no return". + // This can have an adverse impact on caller's code quality as it may have to preserve + // registers across the call. + // TODO-Throughput: We should persist the "no return" information in the runtime + // so we don't need to re-analyze the inlinee all the time. + // + + bool propagate = (m_Observation != InlineObservation::CALLEE_DOES_NOT_RETURN); + + propagate &= LegacyPolicy::PropagateNeverToRuntime(); + + return propagate; +} + #ifdef DEBUG //------------------------------------------------------------------------ diff --git a/src/jit/inlinepolicy.h b/src/jit/inlinepolicy.h index f5773d1ede..b24a8d3ad0 100644 --- a/src/jit/inlinepolicy.h +++ b/src/jit/inlinepolicy.h @@ -156,6 +156,35 @@ protected: bool m_MethodIsMostlyLoadStore :1; }; +// EnhancedLegacyPolicy extends the legacy policy by rejecting +// inlining of methods that never return because they throw. + +class EnhancedLegacyPolicy : public LegacyPolicy +{ +public: + EnhancedLegacyPolicy(Compiler* compiler, bool isPrejitRoot) + : LegacyPolicy(compiler, isPrejitRoot) + , m_IsNoReturn(false) + , m_IsNoReturnKnown(false) + { + // empty + } + + // Policy observations + void NoteBool(InlineObservation obs, bool value) override; + void NoteInt(InlineObservation obs, int value) override; + + // Policy policies + bool PropagateNeverToRuntime() const override; + bool IsLegacyPolicy() const override { return false; } + +protected: + + // Data members + bool m_IsNoReturn :1; + bool m_IsNoReturnKnown :1; +}; + #ifdef DEBUG // RandomPolicy implements a policy that inlines at random. diff --git a/src/jit/jitconfigvalues.h b/src/jit/jitconfigvalues.h index 9969a4e430..44c3676ed7 100644 --- a/src/jit/jitconfigvalues.h +++ b/src/jit/jitconfigvalues.h @@ -203,6 +203,7 @@ CONFIG_STRING(JitNoInlineRange, W("JitNoInlineRange")) CONFIG_STRING(JitInlineReplayFile, W("JitInlineReplayFile")) #endif // defined(DEBUG) || defined(INLINE_DATA) +CONFIG_INTEGER(JitInlinePolicyLegacy, W("JitInlinePolicyLegacy"), 0) CONFIG_INTEGER(JitInlinePolicyModel, W("JitInlinePolicyModel"), 0) #undef CONFIG_INTEGER diff --git a/src/jit/morph.cpp b/src/jit/morph.cpp index 902fce37cc..053cac2e55 100755 --- a/src/jit/morph.cpp +++ b/src/jit/morph.cpp @@ -8033,6 +8033,31 @@ NO_TAIL_CALL: return result; } + if (call->IsNoReturn()) + { + // + // If we know that the call does not return then we can set fgRemoveRestOfBlock + // to remove all subsequent statements and change the call's basic block to BBJ_THROW. + // As a result the compiler won't need to preserve live registers across the call. + // + // This isn't need for tail calls as there shouldn't be any code after the call anyway. + // Besides, the tail call code is part of the epilog and converting the block to + // BBJ_THROW would result in the tail call being dropped as the epilog is generated + // only for BBJ_RETURN blocks. + // + // Currently this doesn't work for non-void callees. Some of the code that handles + // fgRemoveRestOfBlock expects the tree to have GTF_EXCEPT flag set but call nodes + // do not have this flag by default. We could add the flag here but the proper solution + // would be to replace the return expression with a local var node during inlining + // so the rest of the call tree stays in a separate statement. That statement can then + // be removed by fgRemoveRestOfBlock without needing to add GTF_EXCEPT anywhere. + // + + if (!call->IsTailCall() && call->TypeGet() == TYP_VOID) + { + fgRemoveRestOfBlock = true; + } + } return call; } diff --git a/tests/src/JIT/Performance/CodeQuality/Inlining/NoThrowInline.cs b/tests/src/JIT/Performance/CodeQuality/Inlining/NoThrowInline.cs new file mode 100644 index 0000000000..9b689ce376 --- /dev/null +++ b/tests/src/JIT/Performance/CodeQuality/Inlining/NoThrowInline.cs @@ -0,0 +1,75 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using Microsoft.Xunit.Performance; +using System; +using System.Linq; +using System.Runtime.CompilerServices; +using System.Reflection; +using System.Collections.Generic; +using Xunit; + +[assembly: OptimizeForBenchmarks] +[assembly: MeasureInstructionsRetired] + +public static class NoThrowInline +{ +#if DEBUG + public const int Iterations = 1; +#else + public const int Iterations = 100000000; +#endif + + static void ThrowIfNull(string s) + { + if (s == null) + ThrowArgumentNullException(); + } + + static void ThrowArgumentNullException() + { + throw new ArgumentNullException(); + } + + // + // We expect ThrowArgumentNullException to not be inlined into Bench, the throw code is pretty + // large and throws are extremly slow. However, we need to be careful not to degrade the + // non-exception path performance by preserving registers across the call. For this the compiler + // will have to understand that ThrowArgumentNullException never returns and omit the register + // preservation code. + // + // For example, the Bench method below has 4 arguments (all passed in registers on x64) and fairly + // typical argument validation code. If the compiler does not inline ThrowArgumentNullException + // and does not make use of the "no return" information then all 4 register arguments will have + // to be spilled and then reloaded. That would add 8 unnecessary memory accesses. + // + + [MethodImpl(MethodImplOptions.NoInlining)] + static int Bench(string a, string b, string c, string d) + { + ThrowIfNull(a); + ThrowIfNull(b); + ThrowIfNull(c); + ThrowIfNull(d); + + return a.Length + b.Length + c.Length + d.Length; + } + + [Benchmark] + public static void Test() + { + foreach (var iteration in Benchmark.Iterations) + { + using (iteration.StartMeasurement()) + { + Bench("a", "bc", "def", "ghij"); + } + } + } + + public static int Main() + { + return (Bench("a", "bc", "def", "ghij") == 10) ? 100 : -1; + } +} diff --git a/tests/src/JIT/Performance/CodeQuality/Inlining/NoThrowInline.csproj b/tests/src/JIT/Performance/CodeQuality/Inlining/NoThrowInline.csproj new file mode 100644 index 0000000000..ae0576c985 --- /dev/null +++ b/tests/src/JIT/Performance/CodeQuality/Inlining/NoThrowInline.csproj @@ -0,0 +1,47 @@ +<?xml version="1.0" encoding="utf-8"?> +<Project ToolsVersion="12.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003"> + <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" /> + <PropertyGroup> + <Configuration Condition=" '$(Configuration)' == '' ">Debug</Configuration> + <Platform Condition=" '$(Platform)' == '' ">AnyCPU</Platform> + <SchemaVersion>2.0</SchemaVersion> + <ProjectGuid>{95DFC527-4DC1-495E-97D7-E94EE1F7140D}</ProjectGuid> + <OutputType>Exe</OutputType> + <AppDesignerFolder>Properties</AppDesignerFolder> + <FileAlignment>512</FileAlignment> + <ProjectTypeGuids>{786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}</ProjectTypeGuids> + <ReferencePath>$(ProgramFiles)\Common Files\microsoft shared\VSTT\11.0\UITestExtensionPackages</ReferencePath> + <SolutionDir Condition="$(SolutionDir) == '' Or $(SolutionDir) == '*Undefined*'">..\..\</SolutionDir> + <NuGetPackageImportStamp>7a9bfb7d</NuGetPackageImportStamp> + </PropertyGroup> + <!-- Default configurations to help VS understand the configurations --> + <PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Debug|AnyCPU' "> + <DebugType>pdbonly</DebugType> + <Optimize>true</Optimize> + </PropertyGroup> + <PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Release|AnyCPU' "> + <DebugType>pdbonly</DebugType> + <Optimize>true</Optimize> + </PropertyGroup> + <ItemGroup> + <CodeAnalysisDependentAssemblyPaths Condition=" '$(VS100COMNTOOLS)' != '' " Include="$(VS100COMNTOOLS)..\IDE\PrivateAssemblies"> + <Visible>False</Visible> + </CodeAnalysisDependentAssemblyPaths> + </ItemGroup> + <ItemGroup> + <None Include="$(JitPackagesConfigFileDirectory)benchmark\project.json" /> + </ItemGroup> + <ItemGroup> + <Service Include="{82A7F48D-3B50-4B1E-B82E-3ADA8210C358}" /> + </ItemGroup> + <ItemGroup> + <Compile Include="NoThrowInline.cs" /> + </ItemGroup> + <PropertyGroup> + <ProjectJson>$(JitPackagesConfigFileDirectory)benchmark\project.json</ProjectJson> + <ProjectLockJson>$(JitPackagesConfigFileDirectory)benchmark\project.lock.json</ProjectLockJson> + </PropertyGroup> + <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" /> + <PropertyGroup Condition=" '$(MsBuildProjectDirOverride)' != '' "> + </PropertyGroup> +</Project> |