Merge pull request #6337 from AndyAyersMS/CountObservations

Inliner: improve arg observations

4 files changed, 73 insertions, 40 deletions

diff --git a/src/jit/flowgraph.cpp b/src/jit/flowgraph.cpp
index 83c9ad5023..ed2e6f2377 100644
--- 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 0e8739d7fc..aa0573ccf0 100644
--- 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 b3ec841a39..36e1f1b578 100644
--- 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 0f5777a1bf..f5773d1ede 100644
--- 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;