From e62cc0ac118ade0a35905e164334c97d9d63bb38 Mon Sep 17 00:00:00 2001 From: Mike Danes Date: Wed, 3 Aug 2016 22:51:36 +0300 Subject: [PATCH 1/2] Do not inline methods that never return Methods that do not contain return blocks can't ever exit normally, they either throw or loop indefinitely. Inlining is not beneficial in such cases as it increases the code size without providing any speed benefits. In the particular case of throws the inlined code can easily be 10 times larger than the call site. The call to fgMoreThanOneReturnBlock has been replaced with code that does the same thing but also detects the existence of at least one return block. This avoids walking the basic block list twice. Note that BBJ_RETURN blocks are also generated for CEE_JMP. Methods exiting via CEE_JMP instead of CEE_RET will continue to be inlined (assuming they were inlined before this change). --- src/jit/flowgraph.cpp | 38 +++++++++++++- src/jit/gentree.h | 3 ++ src/jit/inline.def | 1 + src/jit/inline.h | 2 +- src/jit/inlinepolicy.cpp | 107 +++++++++++++++++++++++++++++++++++--- src/jit/inlinepolicy.h | 29 +++++++++++ src/jit/jitconfigvalues.h | 1 + src/jit/morph.cpp | 25 +++++++++ 8 files changed, 196 insertions(+), 10 deletions(-) diff --git a/src/jit/flowgraph.cpp b/src/jit/flowgraph.cpp index baf3575529cd..709da122937f 100644 --- a/src/jit/flowgraph.cpp +++ b/src/jit/flowgraph.cpp @@ -5768,14 +5768,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 dd02d9146428..454544143844 100644 --- a/src/jit/gentree.h +++ b/src/jit/gentree.h @@ -2873,6 +2873,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; } @@ -2993,6 +2994,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 f2d90ea15ac0..4e0da3cf2c6a 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 aa0573ccf088..9c9da35d2d5d 100644 --- a/src/jit/inline.h +++ b/src/jit/inline.h @@ -563,7 +563,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 8a8166b13ef0..e8b820303a27 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); } //------------------------------------------------------------------------ @@ -850,6 +853,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(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 f5773d1edeb5..b24a8d3ad09a 100644 --- a/src/jit/inlinepolicy.h +++ b/src/jit/inlinepolicy.h @@ -156,6 +156,35 @@ class LegacyPolicy : public LegalPolicy 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 9969a4e430c4..44c3676ed709 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 5572d8cb3e87..7bb60a7da47e 100755 --- a/src/jit/morph.cpp +++ b/src/jit/morph.cpp @@ -8035,6 +8035,31 @@ GenTreePtr Compiler::fgMorphCall(GenTreeCall* 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; } From e3c3330339630f967bdb27c6bcde089e787b2aeb Mon Sep 17 00:00:00 2001 From: Mike Danes Date: Tue, 5 Jul 2016 23:45:49 +0300 Subject: [PATCH 2/2] Add a throw/inline benchmark --- .../CodeQuality/Inlining/NoThrowInline.cs | 75 +++++++++++++++++++ .../CodeQuality/Inlining/NoThrowInline.csproj | 47 ++++++++++++ 2 files changed, 122 insertions(+) create mode 100644 tests/src/JIT/Performance/CodeQuality/Inlining/NoThrowInline.cs create mode 100644 tests/src/JIT/Performance/CodeQuality/Inlining/NoThrowInline.csproj 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 000000000000..9b689ce37657 --- /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 000000000000..ae0576c985f1 --- /dev/null +++ b/tests/src/JIT/Performance/CodeQuality/Inlining/NoThrowInline.csproj @@ -0,0 +1,47 @@ + + + + + Debug + AnyCPU + 2.0 + {95DFC527-4DC1-495E-97D7-E94EE1F7140D} + Exe + Properties + 512 + {786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC} + $(ProgramFiles)\Common Files\microsoft shared\VSTT\11.0\UITestExtensionPackages + ..\..\ + 7a9bfb7d + + + + pdbonly + true + + + pdbonly + true + + + + False + + + + + + + + + + + + + $(JitPackagesConfigFileDirectory)benchmark\project.json + $(JitPackagesConfigFileDirectory)benchmark\project.lock.json + + + + +