Inliner: improve arg observations

Two related changes to better capture cases where args or constant args
are seen at inline call sites.

On the observation side, the inliner's stack modelling of the callee is
crude and will often overestimate the evaluation stack depth. So when
looking at an opcode that takes just one stack operand (eg BRFALSE) the
inliner's check should be whether the stack has at least one element
instead of checking whether the stack has exactly one element. For
compatibility reasons, the check for exactly one element is still used
when the inline is evaluated via the LegacyPolicy.

On the policy side, instead of keeping a bool flag to note that there
were interesting arg cases, count the number of observations.
LegacyPolicy continues to act as before, checking if count is zero or
nonzero instead of whether the flag was false or true. The count is
available for use in other heuristics and is reported in inline data.

diff --git a/src/jit/flowgraph.cpp b/src/jit/flowgraph.cpp
index 83c9ad5..ed2e6f2 100644 (file)
--- a/src/jit/flowgraph.cpp
+++ b/src/jit/flowgraph.cpp
@@ -4954,39 +4954,54 @@ void Compiler::fgAdjustForAddressExposedOrWrittenThis()
 //
 //    If we're inlining we also look at the argument values supplied by
 //    the caller at this call site.
+//
+//    The crude stack model may overestimate stack depth.
 
 void Compiler::fgObserveInlineConstants(OPCODE opcode, const FgStack& stack, bool isInlining)
 {
     // We should be able to record inline observations.
     assert(compInlineResult != nullptr);
 
-    if (!stack.IsStackTwoDeep())
+    // The stack only has to be 1 deep for BRTRUE/FALSE
+    bool lookForBranchCases = stack.IsStackAtLeastOneDeep();
+
+    if (compInlineResult->UsesLegacyPolicy())
     {
-        // The stack only has to be 1 deep for BRTRUE/FALSE
-        if (stack.IsStackOneDeep())
+        // LegacyPolicy misses cases where the stack is really one
+        // deep but the model says it's two deep. We need to do
+        // likewise to preseve old behavior.
+        lookForBranchCases &= !stack.IsStackTwoDeep();
+    }
+
+    if (lookForBranchCases)
+    {
+        if (opcode == CEE_BRFALSE || opcode == CEE_BRFALSE_S ||
+            opcode == CEE_BRTRUE || opcode == CEE_BRTRUE_S)
         {
-            if (opcode == CEE_BRFALSE || opcode == CEE_BRFALSE_S ||
-                opcode == CEE_BRTRUE || opcode == CEE_BRTRUE_S)
+            unsigned slot0 = stack.GetSlot0();
+            if (FgStack::IsArgument(slot0))
             {
-                unsigned slot0 = stack.GetSlot0();
-                if (FgStack::IsArgument(slot0))
-                {
-                    compInlineResult->Note(InlineObservation::CALLEE_ARG_FEEDS_CONSTANT_TEST);
+                compInlineResult->Note(InlineObservation::CALLEE_ARG_FEEDS_CONSTANT_TEST);
 
-                    if (isInlining)
+                if (isInlining)
+                {
+                    // Check for the double whammy of an incoming constant argument
+                    // feeding a constant test.
+                    unsigned varNum = FgStack::SlotTypeToArgNum(slot0);
+                    if (impInlineInfo->inlArgInfo[varNum].argNode->OperIsConst())
                     {
-                        // Check for the double whammy of an incoming constant argument
-                        // feeding a constant test.
-                        unsigned varNum = FgStack::SlotTypeToArgNum(slot0);
-                        if (impInlineInfo->inlArgInfo[varNum].argNode->OperIsConst())
-                        {
-                            compInlineResult->Note(InlineObservation::CALLSITE_CONSTANT_ARG_FEEDS_TEST);
-                        }
+                        compInlineResult->Note(InlineObservation::CALLSITE_CONSTANT_ARG_FEEDS_TEST);
                     }
                 }
             }
+
+            return;
         }
+    }
 
+    // Remaining cases require at least two things on the stack.
+    if (!stack.IsStackTwoDeep())
+    {
         return;
     }
 

diff --git a/src/jit/inline.h b/src/jit/inline.h
index 0e8739d..aa0573c 100644 (file)
--- a/src/jit/inline.h
+++ b/src/jit/inline.h
@@ -244,6 +244,7 @@ public:
 
     // Policy policies
     virtual bool PropagateNeverToRuntime() const = 0;
+    virtual bool IsLegacyPolicy() const = 0;
 
     // Policy estimates
     virtual int CodeSizeEstimate() = 0;
@@ -451,6 +452,13 @@ public:
         return m_Policy;
     }
 
+    // True if the policy used for this result is (exactly) the legacy
+    // policy.
+    bool UsesLegacyPolicy() const
+    {
+        return m_Policy->IsLegacyPolicy();
+    }
+
     // SetReported indicates that this particular result doesn't need
     // to be reported back to the runtime, either because the runtime
     // already knows, or we aren't actually inlining yet.

diff --git a/src/jit/inlinepolicy.cpp b/src/jit/inlinepolicy.cpp
index b3ec841..36e1f1b 100644 (file)
--- a/src/jit/inlinepolicy.cpp
+++ b/src/jit/inlinepolicy.cpp
@@ -284,7 +284,7 @@ void LegacyPolicy::NoteBool(InlineObservation obs, bool value)
             // LegacyPolicy ignores this for prejit roots.
             if (!m_IsPrejitRoot)
             {
-                m_ArgFeedsConstantTest = value;
+                m_ArgFeedsConstantTest++;
             }
             break;
 
@@ -292,7 +292,7 @@ void LegacyPolicy::NoteBool(InlineObservation obs, bool value)
             // LegacyPolicy ignores this for prejit roots.
             if (!m_IsPrejitRoot)
             {
-                m_ArgFeedsRangeCheck = value;
+                m_ArgFeedsRangeCheck++;
             }
             break;
 
@@ -311,7 +311,7 @@ void LegacyPolicy::NoteBool(InlineObservation obs, bool value)
             // We shouldn't see this for a prejit root since
             // we don't know anything about callers.
             assert(!m_IsPrejitRoot);
-            m_ConstantFeedsConstantTest = value;
+            m_ConstantArgFeedsConstantTest++;
             break;
 
         case InlineObservation::CALLEE_BEGIN_OPCODE_SCAN:
@@ -575,7 +575,7 @@ double LegacyPolicy::DetermineMultiplier()
         JITDUMP("\nInline candidate looks like a wrapper method.  Multiplier increased to %g.", multiplier);
     }
 
-    if (m_ArgFeedsConstantTest)
+    if (m_ArgFeedsConstantTest > 0)
     {
         multiplier += 1.0;
         JITDUMP("\nInline candidate has an arg that feeds a constant test.  Multiplier increased to %g.", multiplier);
@@ -587,13 +587,13 @@ double LegacyPolicy::DetermineMultiplier()
         JITDUMP("\nInline candidate is mostly loads and stores.  Multiplier increased to %g.", multiplier);
     }
 
-    if (m_ArgFeedsRangeCheck)
+    if (m_ArgFeedsRangeCheck > 0)
     {
         multiplier += 0.5;
         JITDUMP("\nInline candidate has arg that feeds range check.  Multiplier increased to %g.", multiplier);
     }
 
-    if (m_ConstantFeedsConstantTest)
+    if (m_ConstantArgFeedsConstantTest > 0)
     {
         multiplier += 3.0;
         JITDUMP("\nInline candidate has const arg that feeds a conditional.  Multiplier increased to %g.", multiplier);
@@ -1164,11 +1164,18 @@ void DiscretionaryPolicy::NoteBool(InlineObservation obs, bool value)
         break;
 
     case InlineObservation::CALLEE_ARG_FEEDS_CONSTANT_TEST:
-        m_ArgFeedsConstantTest = value;
+        assert(value);
+        m_ArgFeedsConstantTest++;
         break;
 
     case InlineObservation::CALLEE_ARG_FEEDS_RANGE_CHECK:
-        m_ArgFeedsRangeCheck = value;
+        assert(value);
+        m_ArgFeedsRangeCheck++;
+        break;
+
+    case InlineObservation::CALLSITE_CONSTANT_ARG_FEEDS_TEST:
+        assert(value);
+        m_ConstantArgFeedsConstantTest++;
         break;
 
     default:
@@ -1701,7 +1708,7 @@ void DiscretionaryPolicy::EstimateCodeSize()
           6.021 * m_CallCount +
          -0.238 * m_IsInstanceCtor +
          -5.357 * m_IsFromPromotableValueClass +
-         -7.901 * m_ConstantFeedsConstantTest +
+         -7.901 * (m_ConstantArgFeedsConstantTest > 0 ? 1 : 0)  +
           0.065 * m_CalleeNativeSizeEstimate;
 
     // Scaled up and reported as an integer value.
@@ -1816,7 +1823,7 @@ void DiscretionaryPolicy::DumpSchema(FILE* file) const
     fprintf(file, ",ArgFeedsConstantTest");
     fprintf(file, ",IsMostlyLoadStore");
     fprintf(file, ",ArgFeedsRangeCheck");
-    fprintf(file, ",ConstantFeedsConstantTest");
+    fprintf(file, ",ConstantArgFeedsConstantTest");
     fprintf(file, ",CalleeNativeSizeEstimate");
     fprintf(file, ",CallsiteNativeSizeEstimate");
     fprintf(file, ",ModelCodeSizeEstimate");
@@ -1888,10 +1895,10 @@ void DiscretionaryPolicy::DumpData(FILE* file) const
     fprintf(file, ",%u", m_IsFromPromotableValueClass ? 1 : 0);
     fprintf(file, ",%u", m_HasSimd ? 1 : 0);
     fprintf(file, ",%u", m_LooksLikeWrapperMethod ? 1 : 0);
-    fprintf(file, ",%u", m_ArgFeedsConstantTest ? 1 : 0);
+    fprintf(file, ",%u", m_ArgFeedsConstantTest);
     fprintf(file, ",%u", m_MethodIsMostlyLoadStore ? 1 : 0);
-    fprintf(file, ",%u", m_ArgFeedsRangeCheck ? 1 : 0);
-    fprintf(file, ",%u", m_ConstantFeedsConstantTest ? 1 : 0);
+    fprintf(file, ",%u", m_ArgFeedsRangeCheck);
+    fprintf(file, ",%u", m_ConstantArgFeedsConstantTest);
     fprintf(file, ",%d", m_CalleeNativeSizeEstimate);
     fprintf(file, ",%d", m_CallsiteNativeSizeEstimate);
     fprintf(file, ",%d", m_ModelCodeSizeEstimate);

diff --git a/src/jit/inlinepolicy.h b/src/jit/inlinepolicy.h
index 0f5777a..f5773d1 100644 (file)
--- a/src/jit/inlinepolicy.h
+++ b/src/jit/inlinepolicy.h
@@ -82,23 +82,23 @@ public:
         : LegalPolicy(isPrejitRoot)
         , m_RootCompiler(compiler)
         , m_StateMachine(nullptr)
+        , m_Multiplier(0.0)
         , m_CodeSize(0)
         , m_CallsiteFrequency(InlineCallsiteFrequency::UNUSED)
         , m_InstructionCount(0)
         , m_LoadStoreCount(0)
+        , m_ArgFeedsConstantTest(0)
+        , m_ArgFeedsRangeCheck(0)
+        , m_ConstantArgFeedsConstantTest(0)
         , m_CalleeNativeSizeEstimate(0)
         , m_CallsiteNativeSizeEstimate(0)
-        , m_Multiplier(0.0)
         , m_IsForceInline(false)
         , m_IsForceInlineKnown(false)
         , m_IsInstanceCtor(false)
         , m_IsFromPromotableValueClass(false)
         , m_HasSimd(false)
         , m_LooksLikeWrapperMethod(false)
-        , m_ArgFeedsConstantTest(false)
         , m_MethodIsMostlyLoadStore(false)
-        , m_ArgFeedsRangeCheck(false)
-        , m_ConstantFeedsConstantTest(false)
     {
         // empty
     }
@@ -113,6 +113,7 @@ public:
 
     // Policy policies
     bool PropagateNeverToRuntime() const override { return true; }
+    bool IsLegacyPolicy() const override { return true; }
 
     // Policy estimates
     int CodeSizeEstimate() override;
@@ -136,23 +137,23 @@ protected:
     // Data members
     Compiler*               m_RootCompiler;                      // root compiler instance
     CodeSeqSM*              m_StateMachine;
+    double                  m_Multiplier;
     unsigned                m_CodeSize;
     InlineCallsiteFrequency m_CallsiteFrequency;
     unsigned                m_InstructionCount;
     unsigned                m_LoadStoreCount;
+    unsigned                m_ArgFeedsConstantTest;
+    unsigned                m_ArgFeedsRangeCheck;
+    unsigned                m_ConstantArgFeedsConstantTest;
     int                     m_CalleeNativeSizeEstimate;
     int                     m_CallsiteNativeSizeEstimate;
-    double                  m_Multiplier;
     bool                    m_IsForceInline :1;
     bool                    m_IsForceInlineKnown :1;
     bool                    m_IsInstanceCtor :1;
     bool                    m_IsFromPromotableValueClass :1;
     bool                    m_HasSimd :1;
     bool                    m_LooksLikeWrapperMethod :1;
-    bool                    m_ArgFeedsConstantTest :1;
     bool                    m_MethodIsMostlyLoadStore :1;
-    bool                    m_ArgFeedsRangeCheck :1;
-    bool                    m_ConstantFeedsConstantTest :1;
 };
 
 #ifdef DEBUG
@@ -177,6 +178,7 @@ public:
 
     // Policy policies
     bool PropagateNeverToRuntime() const override { return true; }
+    bool IsLegacyPolicy() const override { return false; }
 
     // Policy estimates
     int CodeSizeEstimate() override
@@ -200,8 +202,8 @@ private:
 
 // DiscretionaryPolicy is a variant of the legacy policy.  It differs
 // in that there is no ALWAYS_INLINE class, there is no IL size limit,
-// and in prejit mode, discretionary failures do not set the "NEVER"
-// inline bit.
+// it does not try and maintain legacy compatabilty, and in prejit mode,
+// discretionary failures do not set the "NEVER" inline bit.
 //
 // It is useful for gathering data about inline costs.
 
@@ -218,6 +220,7 @@ public:
 
     // Policy policies
     bool PropagateNeverToRuntime() const override;
+    bool IsLegacyPolicy() const override { return false; }
 
     // Policy determinations
     void DetermineProfitability(CORINFO_METHOD_INFO* methodInfo) override;