Inliner: updates to RandomPolicy (#8128)

Fix issues with RandomPolicy setup in release builds with -DINLINE_DATA.

Reparent RandomPolicy on top of DiscretionaryPolicy to enable inline
data dumps from random inline runs. Remove some now-unneeded overrides.

Add a full dump mode to JitInlineDumpData that reports the inliner-visible
data for all inlines, as opposed to inliner-visible and post-inline
data for just the most recent inline. Small mods to the dumper code to
adjust comma placement for this new mode.

Have the RandomPolicy make the full set of profitability observations
for each accepted inline so they can be dumped.

3 files changed, 87 insertions, 141 deletions

diff --git a/src/jit/inline.cpp b/src/jit/inline.cpp
index 8ebaa9e7a8..05fcf1c6b9 100644
--- a/src/jit/inline.cpp
+++ b/src/jit/inline.cpp
@@ -447,7 +447,7 @@ void InlineContext::DumpData(unsigned indent)
     else if (m_Success)
     {
         const char* inlineReason = InlGetObservationString(m_Observation);
-        printf("%*s%u,\"%s\",\"%s\"", indent, "", m_Ordinal, inlineReason, calleeName);
+        printf("%*s%u,\"%s\",\"%s\",", indent, "", m_Ordinal, inlineReason, calleeName);
         m_Policy->DumpData(jitstdout);
         printf("\n");
     }
@@ -500,14 +500,25 @@ void InlineContext::DumpXml(FILE* file, unsigned indent)
         fprintf(file, "%*s<Offset>%u</Offset>\n", indent + 2, "", offset);
         fprintf(file, "%*s<Reason>%s</Reason>\n", indent + 2, "", inlineReason);
 
-        // Optionally, dump data about the last inline
-        if ((JitConfig.JitInlineDumpData() != 0) && (this == m_InlineStrategy->GetLastContext()))
+        // Optionally, dump data about the inline
+        const int dumpDataSetting = JitConfig.JitInlineDumpData();
+
+        // JitInlineDumpData=1 -- dump data plus deltas for last inline only
+        if ((dumpDataSetting == 1) && (this == m_InlineStrategy->GetLastContext()))
         {
             fprintf(file, "%*s<Data>", indent + 2, "");
             m_InlineStrategy->DumpDataContents(file);
             fprintf(file, "</Data>\n");
         }
 
+        // JitInlineDumpData=2 -- dump data for all inlines, no deltas
+        if ((dumpDataSetting == 2) && (m_Policy != nullptr))
+        {
+            fprintf(file, "%*s<Data>", indent + 2, "");
+            m_Policy->DumpData(file);
+            fprintf(file, "</Data>\n");
+        }
+
         newIndent = indent + 2;
     }
 
@@ -1383,7 +1394,7 @@ void InlineStrategy::DumpDataHeader(FILE* file)
 void InlineStrategy::DumpDataSchema(FILE* file)
 {
     DumpDataEnsurePolicyIsSet();
-    fprintf(file, "Method,Version,HotSize,ColdSize,JitTime,SizeEstimate,TimeEstimate");
+    fprintf(file, "Method,Version,HotSize,ColdSize,JitTime,SizeEstimate,TimeEstimate,");
     m_LastSuccessfulPolicy->DumpSchema(file);
 }
 
@@ -1419,7 +1430,7 @@ void InlineStrategy::DumpDataContents(FILE* file)
         microsecondsSpentJitting = (unsigned)((counts / countsPerSec) * 1000 * 1000);
     }
 
-    fprintf(file, "%08X,%u,%u,%u,%u,%d,%d", currentMethodToken, m_InlineCount, info.compTotalHotCodeSize,
+    fprintf(file, "%08X,%u,%u,%u,%u,%d,%d,", currentMethodToken, m_InlineCount, info.compTotalHotCodeSize,
             info.compTotalColdCodeSize, microsecondsSpentJitting, m_CurrentSizeEstimate / 10, m_CurrentTimeEstimate);
     m_LastSuccessfulPolicy->DumpData(file);
 }
@@ -1456,10 +1467,22 @@ void InlineStrategy::DumpXml(FILE* file, unsigned indent)
         fprintf(file, "<InlineForest>\n");
         fprintf(file, "<Policy>%s</Policy>\n", m_LastSuccessfulPolicy->GetName());
 
-        if (JitConfig.JitInlineDumpData() != 0)
+        const int dumpDataSetting = JitConfig.JitInlineDumpData();
+        if (dumpDataSetting != 0)
         {
             fprintf(file, "<DataSchema>");
-            DumpDataSchema(file);
+
+            if (dumpDataSetting == 1)
+            {
+                // JitInlineDumpData=1 -- dump schema for data plus deltas
+                DumpDataSchema(file);
+            }
+            else if (dumpDataSetting == 2)
+            {
+                // JitInlineDumpData=2 -- dump schema for data only
+                m_LastSuccessfulPolicy->DumpSchema(file);
+            }
+
             fprintf(file, "</DataSchema>\n");
         }
 
@@ -1600,11 +1623,16 @@ CLRRandom* InlineStrategy::GetRandom()
     if (m_Random == nullptr)
     {
         int externalSeed = 0;
+
+#ifdef DEBUG
+
         if (m_Compiler->compRandomInlineStress())
         {
             externalSeed = getJitStressLevel();
         }
 
+#endif // DEBUG
+
         int randomPolicyFlag = JitConfig.JitInlinePolicyRandom();
         if (randomPolicyFlag != 0)
         {
diff --git a/src/jit/inlinepolicy.cpp b/src/jit/inlinepolicy.cpp
index a909845177..d108b987e0 100644
--- a/src/jit/inlinepolicy.cpp
+++ b/src/jit/inlinepolicy.cpp
@@ -27,21 +27,22 @@
 InlinePolicy* InlinePolicy::GetPolicy(Compiler* compiler, bool isPrejitRoot)
 {
 
-#ifdef DEBUG
+#if defined(DEBUG) || defined(INLINE_DATA)
 
-    // Optionally install the RandomPolicy.
+#if defined(DEBUG)
     const bool useRandomPolicyForStress = compiler->compRandomInlineStress();
-    const bool useRandomPolicy          = (JitConfig.JitInlinePolicyRandom() != 0);
+#else
+    const bool useRandomPolicyForStress = false;
+#endif // defined(DEBUG)
 
+    const bool useRandomPolicy = (JitConfig.JitInlinePolicyRandom() != 0);
+
+    // Optionally install the RandomPolicy.
     if (useRandomPolicyForStress || useRandomPolicy)
     {
         return new (compiler, CMK_Inlining) RandomPolicy(compiler, isPrejitRoot);
     }
 
-#endif // DEBUG
-
-#if defined(DEBUG) || defined(INLINE_DATA)
-
     // Optionally install the ReplayPolicy.
     bool useReplayPolicy = JitConfig.JitInlinePolicyReplay() != 0;
 
@@ -982,78 +983,12 @@ bool EnhancedLegacyPolicy::PropagateNeverToRuntime() const
 //    compiler -- compiler instance doing the inlining (root compiler)
 //    isPrejitRoot -- true if this compiler is prejitting the root method
 
-RandomPolicy::RandomPolicy(Compiler* compiler, bool isPrejitRoot)
-    : LegalPolicy(isPrejitRoot)
-    , m_RootCompiler(compiler)
-    , m_Random(nullptr)
-    , m_CodeSize(0)
-    , m_IsForceInline(false)
-    , m_IsForceInlineKnown(false)
+RandomPolicy::RandomPolicy(Compiler* compiler, bool isPrejitRoot) : DiscretionaryPolicy(compiler, isPrejitRoot)
 {
     m_Random = compiler->m_inlineStrategy->GetRandom();
 }
 
 //------------------------------------------------------------------------
-// NoteSuccess: handle finishing all the inlining checks successfully
-
-void RandomPolicy::NoteSuccess()
-{
-    assert(InlDecisionIsCandidate(m_Decision));
-    m_Decision = InlineDecision::SUCCESS;
-}
-
-//------------------------------------------------------------------------
-// NoteBool: handle a boolean observation with non-fatal impact
-//
-// Arguments:
-//    obs      - the current obsevation
-//    value    - the value of the observation
-void RandomPolicy::NoteBool(InlineObservation obs, bool value)
-{
-    // Check the impact
-    InlineImpact impact = InlGetImpact(obs);
-
-    // As a safeguard, all fatal impact must be
-    // reported via NoteFatal.
-    assert(impact != InlineImpact::FATAL);
-
-    // Handle most information here
-    bool isInformation = (impact == InlineImpact::INFORMATION);
-    bool propagate     = !isInformation;
-
-    if (isInformation)
-    {
-        switch (obs)
-        {
-            case InlineObservation::CALLEE_IS_FORCE_INLINE:
-                // The RandomPolicy still honors force inlines.
-                //
-                // We may make the force-inline observation more than
-                // once.  All observations should agree.
-                assert(!m_IsForceInlineKnown || (m_IsForceInline == value));
-                m_IsForceInline      = value;
-                m_IsForceInlineKnown = true;
-                break;
-
-            case InlineObservation::CALLEE_HAS_SWITCH:
-            case InlineObservation::CALLEE_UNSUPPORTED_OPCODE:
-                // Pass these on, they should cause inlining to fail.
-                propagate = true;
-                break;
-
-            default:
-                // Ignore the remainder for now
-                break;
-        }
-    }
-
-    if (propagate)
-    {
-        NoteInternal(obs);
-    }
-}
-
-//------------------------------------------------------------------------
 // NoteInt: handle an observed integer value
 //
 // Arguments:
@@ -1064,7 +999,6 @@ void RandomPolicy::NoteInt(InlineObservation obs, int value)
 {
     switch (obs)
     {
-
         case InlineObservation::CALLEE_IL_CODE_SIZE:
         {
             assert(m_IsForceInlineKnown);
@@ -1086,7 +1020,8 @@ void RandomPolicy::NoteInt(InlineObservation obs, int value)
         }
 
         default:
-            // Ignore all other information
+            // Defer to superclass for all other information
+            DiscretionaryPolicy::NoteInt(obs, value);
             break;
     }
 }
@@ -1119,6 +1054,16 @@ void RandomPolicy::DetermineProfitability(CORINFO_METHOD_INFO* methodInfo)
         }
     }
 
+    // If we're also dumping inline data, make additional observations
+    // based on the method info, and estimate code size and perf
+    // impact, so that the reports have the necessary data.
+    if (JitConfig.JitInlineDumpData() != 0)
+    {
+        MethodInfoObservations(methodInfo);
+        EstimateCodeSize();
+        EstimatePerformanceImpact();
+    }
+
     // Use a probability curve that roughly matches the observed
     // behavior of the LegacyPolicy. That way we're inlining
     // differently but not creating enormous methods.
@@ -1337,7 +1282,6 @@ void DiscretionaryPolicy::NoteInt(InlineObservation obs, int value)
 {
     switch (obs)
     {
-
         case InlineObservation::CALLEE_IL_CODE_SIZE:
             // Override how code size is handled
             {
@@ -1911,7 +1855,7 @@ int DiscretionaryPolicy::CodeSizeEstimate()
 
 void DiscretionaryPolicy::DumpSchema(FILE* file) const
 {
-    fprintf(file, ",ILSize");
+    fprintf(file, "ILSize");
     fprintf(file, ",CallsiteFrequency");
     fprintf(file, ",InstructionCount");
     fprintf(file, ",LoadStoreCount");
@@ -1993,7 +1937,7 @@ void DiscretionaryPolicy::DumpSchema(FILE* file) const
 
 void DiscretionaryPolicy::DumpData(FILE* file) const
 {
-    fprintf(file, ",%u", m_CodeSize);
+    fprintf(file, "%u", m_CodeSize);
     fprintf(file, ",%u", m_CallsiteFrequency);
     fprintf(file, ",%u", m_InstructionCount);
     fprintf(file, ",%u", m_LoadStoreCount);
diff --git a/src/jit/inlinepolicy.h b/src/jit/inlinepolicy.h
index c010a98eec..3239dcbe89 100644
--- a/src/jit/inlinepolicy.h
+++ b/src/jit/inlinepolicy.h
@@ -198,57 +198,6 @@ protected:
     bool m_IsNoReturnKnown : 1;
 };
 
-#if defined(DEBUG) || defined(INLINE_DATA)
-
-// RandomPolicy implements a policy that inlines at random.
-// It is mostly useful for stress testing.
-
-class RandomPolicy : public LegalPolicy
-{
-public:
-    // Construct a RandomPolicy
-    RandomPolicy(Compiler* compiler, bool isPrejitRoot);
-
-    // Policy observations
-    void NoteSuccess() override;
-    void NoteBool(InlineObservation obs, bool value) override;
-    void NoteInt(InlineObservation obs, int value) override;
-
-    // Policy determinations
-    void DetermineProfitability(CORINFO_METHOD_INFO* methodInfo) override;
-
-    // Policy policies
-    bool PropagateNeverToRuntime() const override
-    {
-        return true;
-    }
-    bool IsLegacyPolicy() const override
-    {
-        return false;
-    }
-
-    // Policy estimates
-    int CodeSizeEstimate() override
-    {
-        return 0;
-    }
-
-    const char* GetName() const override
-    {
-        return "RandomPolicy";
-    }
-
-private:
-    // Data members
-    Compiler*  m_RootCompiler;
-    CLRRandom* m_Random;
-    unsigned   m_CodeSize;
-    bool       m_IsForceInline : 1;
-    bool       m_IsForceInlineKnown : 1;
-};
-
-#endif // defined(DEBUG) || defined(INLINE_DATA)
-
 // DiscretionaryPolicy is a variant of the enhanced legacy policy.  It
 // differs in that there is no ALWAYS_INLINE class, there is no IL
 // size limit, it does not try and maintain legacy compatabilty, and
@@ -269,10 +218,6 @@ public:
 
     // Policy policies
     bool PropagateNeverToRuntime() const override;
-    bool IsLegacyPolicy() const override
-    {
-        return false;
-    }
 
     // Policy determinations
     void DetermineProfitability(CORINFO_METHOD_INFO* methodInfo) override;
@@ -386,6 +331,35 @@ public:
 
 #if defined(DEBUG) || defined(INLINE_DATA)
 
+// RandomPolicy implements a policy that inlines at random.
+// It is mostly useful for stress testing.
+
+class RandomPolicy : public DiscretionaryPolicy
+{
+public:
+    // Construct a RandomPolicy
+    RandomPolicy(Compiler* compiler, bool isPrejitRoot);
+
+    // Policy observations
+    void NoteInt(InlineObservation obs, int value) override;
+
+    // Policy determinations
+    void DetermineProfitability(CORINFO_METHOD_INFO* methodInfo) override;
+
+    const char* GetName() const override
+    {
+        return "RandomPolicy";
+    }
+
+private:
+    // Data members
+    CLRRandom* m_Random;
+};
+
+#endif // defined(DEBUG) || defined(INLINE_DATA)
+
+#if defined(DEBUG) || defined(INLINE_DATA)
+
 // FullPolicy is an experimental policy that will always inline if
 // possible, subject to externally settable depth and size limits.
 //