Merge pull request #3933 from AndyAyersMS/RefactorPolicy

Inliner: refactor policies to extract common legality portion

2 files changed, 168 insertions, 274 deletions

diff --git a/src/jit/inlinepolicy.cpp b/src/jit/inlinepolicy.cpp
index a53b3cd68e..ec18179fca 100644
--- a/src/jit/inlinepolicy.cpp
+++ b/src/jit/inlinepolicy.cpp
@@ -60,6 +60,132 @@ InlinePolicy* InlinePolicy::GetPolicy(Compiler* compiler, bool isPrejitRoot)
 }
 
 //------------------------------------------------------------------------
+// NoteFatal: handle an observation with fatal impact
+//
+// Arguments:
+//    obs      - the current obsevation
+
+void LegalPolicy::NoteFatal(InlineObservation obs)
+{
+    // As a safeguard, all fatal impact must be
+    // reported via noteFatal.
+    assert(InlGetImpact(obs) == InlineImpact::FATAL);
+    NoteInternal(obs);
+    assert(InlDecisionIsFailure(m_Decision));
+}
+
+//------------------------------------------------------------------------
+// NoteInternal: helper for handling an observation
+//
+// Arguments:
+//    obs      - the current obsevation
+
+void LegalPolicy::NoteInternal(InlineObservation obs)
+{
+    // Note any INFORMATION that reaches here will now cause failure.
+    // Non-fatal INFORMATION observations must be handled higher up.
+    InlineTarget target = InlGetTarget(obs);
+
+    if (target == InlineTarget::CALLEE)
+    {
+        this->SetNever(obs);
+    }
+    else
+    {
+        this->SetFailure(obs);
+    }
+}
+
+//------------------------------------------------------------------------
+// SetFailure: helper for setting a failing decision
+//
+// Arguments:
+//    obs      - the current obsevation
+
+void LegalPolicy::SetFailure(InlineObservation obs)
+{
+    // Expect a valid observation
+    assert(InlIsValidObservation(obs));
+
+    switch (m_Decision)
+    {
+    case InlineDecision::FAILURE:
+        // Repeated failure only ok if evaluating a prejit root
+        // (since we can't fail fast because we're not inlining)
+        // or if inlining and the observation is CALLSITE_TOO_MANY_LOCALS
+        // (since we can't fail fast from lvaGrabTemp).
+        assert(m_IsPrejitRoot ||
+               (obs == InlineObservation::CALLSITE_TOO_MANY_LOCALS));
+        break;
+    case InlineDecision::UNDECIDED:
+    case InlineDecision::CANDIDATE:
+        m_Decision = InlineDecision::FAILURE;
+        m_Observation = obs;
+        break;
+    default:
+        // SUCCESS, NEVER, or ??
+        assert(!"Unexpected m_Decision");
+        unreached();
+    }
+}
+
+//------------------------------------------------------------------------
+// SetNever: helper for setting a never decision
+//
+// Arguments:
+//    obs      - the current obsevation
+
+void LegalPolicy::SetNever(InlineObservation obs)
+{
+    // Expect a valid observation
+    assert(InlIsValidObservation(obs));
+
+    switch (m_Decision)
+    {
+    case InlineDecision::NEVER:
+        // Repeated never only ok if evaluating a prejit root
+        assert(m_IsPrejitRoot);
+        break;
+    case InlineDecision::UNDECIDED:
+    case InlineDecision::CANDIDATE:
+        m_Decision = InlineDecision::NEVER;
+        m_Observation = obs;
+        break;
+    default:
+        // SUCCESS, FAILURE or ??
+        assert(!"Unexpected m_Decision");
+        unreached();
+    }
+}
+
+//------------------------------------------------------------------------
+// SetCandidate: helper updating candidacy
+//
+// Arguments:
+//    obs      - the current obsevation
+//
+// Note:
+//    Candidate observations are handled here. If the inline has already
+//    failed, they're ignored. If there's already a candidate reason,
+//    this new reason trumps it.
+
+void LegalPolicy::SetCandidate(InlineObservation obs)
+{
+    // Ignore if this inline is going to fail.
+    if (InlDecisionIsFailure(m_Decision))
+    {
+        return;
+    }
+
+    // We should not have declared success yet.
+    assert(!InlDecisionIsSuccess(m_Decision));
+
+    // Update, overriding any previous candidacy.
+    m_Decision = InlineDecision::CANDIDATE;
+    m_Observation = obs;
+}
+
+//------------------------------------------------------------------------
 // NoteSuccess: handle finishing all the inlining checks successfully
 
 void LegacyPolicy::NoteSuccess()
@@ -203,21 +329,6 @@ void LegacyPolicy::NoteBool(InlineObservation obs, bool value)
 }
 
 //------------------------------------------------------------------------
-// NoteFatal: handle an observation with fatal impact
-//
-// Arguments:
-//    obs      - the current obsevation
-
-void LegacyPolicy::NoteFatal(InlineObservation obs)
-{
-    // As a safeguard, all fatal impact must be
-    // reported via noteFatal.
-    assert(InlGetImpact(obs) == InlineImpact::FATAL);
-    NoteInternal(obs);
-    assert(InlDecisionIsFailure(m_Decision));
-}
-
-//------------------------------------------------------------------------
 // NoteInt: handle an observed integer value
 //
 // Arguments:
@@ -343,116 +454,6 @@ void LegacyPolicy::NoteInt(InlineObservation obs, int value)
     }
 }
 
-//------------------------------------------------------------------------
-// NoteInternal: helper for handling an observation
-//
-// Arguments:
-//    obs      - the current obsevation
-
-void LegacyPolicy::NoteInternal(InlineObservation obs)
-{
-    // Note any INFORMATION that reaches here will now cause failure.
-    // Non-fatal INFORMATION observations must be handled higher up.
-    InlineTarget target = InlGetTarget(obs);
-
-    if (target == InlineTarget::CALLEE)
-    {
-        this->SetNever(obs);
-    }
-    else
-    {
-        this->SetFailure(obs);
-    }
-}
-
-//------------------------------------------------------------------------
-// SetFailure: helper for setting a failing decision
-//
-// Arguments:
-//    obs      - the current obsevation
-
-void LegacyPolicy::SetFailure(InlineObservation obs)
-{
-    // Expect a valid observation
-    assert(InlIsValidObservation(obs));
-
-    switch (m_Decision)
-    {
-    case InlineDecision::FAILURE:
-        // Repeated failure only ok if evaluating a prejit root
-        // (since we can't fail fast because we're not inlining)
-        // or if inlining and the observation is CALLSITE_TOO_MANY_LOCALS
-        // (since we can't fail fast from lvaGrabTemp).
-        assert(m_IsPrejitRoot ||
-               (obs == InlineObservation::CALLSITE_TOO_MANY_LOCALS));
-        break;
-    case InlineDecision::UNDECIDED:
-    case InlineDecision::CANDIDATE:
-        m_Decision = InlineDecision::FAILURE;
-        m_Observation = obs;
-        break;
-    default:
-        // SUCCESS, NEVER, or ??
-        assert(!"Unexpected m_Decision");
-        unreached();
-    }
-}
-
-//------------------------------------------------------------------------
-// SetNever: helper for setting a never decision
-//
-// Arguments:
-//    obs      - the current obsevation
-
-void LegacyPolicy::SetNever(InlineObservation obs)
-{
-    // Expect a valid observation
-    assert(InlIsValidObservation(obs));
-
-    switch (m_Decision)
-    {
-    case InlineDecision::NEVER:
-        // Repeated never only ok if evaluating a prejit root
-        assert(m_IsPrejitRoot);
-        break;
-    case InlineDecision::UNDECIDED:
-    case InlineDecision::CANDIDATE:
-        m_Decision = InlineDecision::NEVER;
-        m_Observation = obs;
-        break;
-    default:
-        // SUCCESS, FAILURE or ??
-        assert(!"Unexpected m_Decision");
-        unreached();
-    }
-}
-
-//------------------------------------------------------------------------
-// SetCandidate: helper updating candidacy
-//
-// Arguments:
-//    obs      - the current obsevation
-//
-// Note:
-//    Candidate observations are handled here. If the inline has already
-//    failed, they're ignored. If there's already a candidate reason,
-//    this new reason trumps it.
-
-void LegacyPolicy::SetCandidate(InlineObservation obs)
-{
-    // Ignore if this inline is going to fail.
-    if (InlDecisionIsFailure(m_Decision))
-    {
-        return;
-    }
-
-    // We should not have declared success yet.
-    assert(!InlDecisionIsSuccess(m_Decision));
-
-    // Update, overriding any previous candidacy.
-    m_Decision = InlineDecision::CANDIDATE;
-    m_Observation = obs;
-}
 
 //------------------------------------------------------------------------
 // DetermineMultiplier: determine benefit multiplier for this inline
@@ -739,7 +740,7 @@ void LegacyPolicy::DetermineProfitability(CORINFO_METHOD_INFO* methodInfo)
 //    seed -- seed value for the random number generator
 
 RandomPolicy::RandomPolicy(Compiler* compiler, bool isPrejitRoot, unsigned seed)
-    : InlinePolicy(isPrejitRoot)
+    : LegalPolicy(isPrejitRoot)
     , m_RootCompiler(compiler)
     , m_Random(nullptr)
     , m_CodeSize(0)
@@ -823,21 +824,6 @@ void RandomPolicy::NoteBool(InlineObservation obs, bool value)
 }
 
 //------------------------------------------------------------------------
-// NoteFatal: handle an observation with fatal impact
-//
-// Arguments:
-//    obs      - the current obsevation
-
-void RandomPolicy::NoteFatal(InlineObservation obs)
-{
-    // As a safeguard, all fatal impact must be
-    // reported via noteFatal.
-    assert(InlGetImpact(obs) == InlineImpact::FATAL);
-    NoteInternal(obs);
-    assert(InlDecisionIsFailure(m_Decision));
-}
-
-//------------------------------------------------------------------------
 // NoteInt: handle an observed integer value
 //
 // Arguments:
@@ -876,117 +862,6 @@ void RandomPolicy::NoteInt(InlineObservation obs, int value)
 }
 
 //------------------------------------------------------------------------
-// NoteInternal: helper for handling an observation
-//
-// Arguments:
-//    obs      - the current obsevation
-
-void RandomPolicy::NoteInternal(InlineObservation obs)
-{
-    // Note any INFORMATION that reaches here will now cause failure.
-    // Non-fatal INFORMATION observations must be handled higher up.
-    InlineTarget target = InlGetTarget(obs);
-
-    if (target == InlineTarget::CALLEE)
-    {
-        this->SetNever(obs);
-    }
-    else
-    {
-        this->SetFailure(obs);
-    }
-}
-
-//------------------------------------------------------------------------
-// SetFailure: helper for setting a failing decision
-//
-// Arguments:
-//    obs      - the current obsevation
-
-void RandomPolicy::SetFailure(InlineObservation obs)
-{
-    // Expect a valid observation
-    assert(InlIsValidObservation(obs));
-
-    switch (m_Decision)
-    {
-    case InlineDecision::FAILURE:
-        // Repeated failure only ok if evaluating a prejit root
-        // (since we can't fail fast because we're not inlining)
-        // or if inlining and the observation is CALLSITE_TOO_MANY_LOCALS
-        // (since we can't fail fast from lvaGrabTemp).
-        assert(m_IsPrejitRoot ||
-               (obs == InlineObservation::CALLSITE_TOO_MANY_LOCALS));
-        break;
-    case InlineDecision::UNDECIDED:
-    case InlineDecision::CANDIDATE:
-        m_Decision = InlineDecision::FAILURE;
-        m_Observation = obs;
-        break;
-    default:
-        // SUCCESS, NEVER, or ??
-        assert(!"Unexpected m_Decision");
-        unreached();
-    }
-}
-
-//------------------------------------------------------------------------
-// SetNever: helper for setting a never decision
-//
-// Arguments:
-//    obs      - the current obsevation
-
-void RandomPolicy::SetNever(InlineObservation obs)
-{
-    // Expect a valid observation
-    assert(InlIsValidObservation(obs));
-
-    switch (m_Decision)
-    {
-    case InlineDecision::NEVER:
-        // Repeated never only ok if evaluating a prejit root
-        assert(m_IsPrejitRoot);
-        break;
-    case InlineDecision::UNDECIDED:
-    case InlineDecision::CANDIDATE:
-        m_Decision = InlineDecision::NEVER;
-        m_Observation = obs;
-        break;
-    default:
-        // SUCCESS, FAILURE or ??
-        assert(!"Unexpected m_Decision");
-        unreached();
-    }
-}
-
-//------------------------------------------------------------------------
-// SetCandidate: helper updating candidacy
-//
-// Arguments:
-//    obs      - the current obsevation
-//
-// Note:
-//    Candidate observations are handled here. If the inline has already
-//    failed, they're ignored. If there's already a candidate reason,
-//    this new reason trumps it.
-
-void RandomPolicy::SetCandidate(InlineObservation obs)
-{
-    // Ignore if this inline is going to fail.
-    if (InlDecisionIsFailure(m_Decision))
-    {
-        return;
-    }
-
-    // We should not have declared success yet.
-    assert(!InlDecisionIsSuccess(m_Decision));
-
-    // Update, overriding any previous candidacy.
-    m_Decision = InlineDecision::CANDIDATE;
-    m_Observation = obs;
-}
-
-//------------------------------------------------------------------------
 // DetermineProfitability: determine if this inline is profitable
 //
 // Arguments:
diff --git a/src/jit/inlinepolicy.h b/src/jit/inlinepolicy.h
index 13673bcc3e..0d192a15ee 100644
--- a/src/jit/inlinepolicy.h
+++ b/src/jit/inlinepolicy.h
@@ -9,7 +9,8 @@
 //
 // -- CLASSES --
 //
-// LegacyPolicy        - policy to provide legacy inline behavior
+// LegalPolicy         - partial class providing common legality checks
+// LegacyPolicy        - policy that provides legacy inline behavior
 // RandomPolicy        - randomized inlining
 // DiscretionaryPolicy - legacy variant with uniform size policy
 
@@ -19,29 +20,59 @@
 #include "jit.h"
 #include "inline.h"
 
-class CodeSeqSM;
-
-// LegacyPolicy implements the inlining policy used by the jit in its
-// initial release.
+// LegalPolicy is a partial policy that encapsulates the common
+// legality and ability checks the inliner must make.
 //
-// Generally speaking, the legacy policy expects the inlining attempt
+// Generally speaking, the legal policy expects the inlining attempt
 // to fail fast when a fatal or equivalent observation is made. So
 // once an observation causes failure, no more observations are
 // expected. However for the prejit scan case (where the jit is not
 // actually inlining, but is assessing a method's general
-// inlinability) the legacy policy allows multiple failing
+// inlinability) the legal policy allows multiple failing
 // observations provided they have the same impact. Only the first
 // observation that puts the policy into a failing state is
 // remembered. Transitions from failing states to candidate or success
 // states are not allowed.
 
-class LegacyPolicy : public InlinePolicy
+class LegalPolicy : public InlinePolicy
+{
+
+public:
+
+    // Constructor
+    LegalPolicy(bool isPrejitRoot)
+        : InlinePolicy(isPrejitRoot)
+    {
+        // empty
+    }
+
+    // Handle an observation that must cause inlining to fail.
+    void NoteFatal(InlineObservation obs) override;
+
+protected:
+
+    // Helper methods
+    void NoteInternal(InlineObservation obs);
+    void SetCandidate(InlineObservation obs);
+    void SetFailure(InlineObservation obs);
+    void SetNever(InlineObservation obs);
+};
+
+// Forward declaration for the state machine class used by the
+// LegacyPolicy
+
+class CodeSeqSM;
+
+// LegacyPolicy implements the inlining policy used by the jit in its
+// initial release.
+
+class LegacyPolicy : public LegalPolicy
 {
 public:
 
     // Construct a LegacyPolicy
     LegacyPolicy(Compiler* compiler, bool isPrejitRoot)
-        : InlinePolicy(isPrejitRoot)
+        : LegalPolicy(isPrejitRoot)
         , m_RootCompiler(compiler)
         , m_StateMachine(nullptr)
         , m_CodeSize(0)
@@ -68,7 +99,6 @@ public:
     // Policy observations
     void NoteSuccess() override;
     void NoteBool(InlineObservation obs, bool value) override;
-    void NoteFatal(InlineObservation obs) override;
     void NoteInt(InlineObservation obs, int value) override;
 
     // Policy determinations
@@ -86,10 +116,6 @@ public:
 protected:
 
     // Helper methods
-    void NoteInternal(InlineObservation obs);
-    void SetCandidate(InlineObservation obs);
-    void SetFailure(InlineObservation obs);
-    void SetNever(InlineObservation obs);
     double DetermineMultiplier();
     int DetermineNativeSizeEstimate();
     int DetermineCallsiteNativeSizeEstimate(CORINFO_METHOD_INFO* methodInfo);
@@ -124,7 +150,7 @@ protected:
 // RandomPolicy implements a policy that inlines at random.
 // It is mostly useful for stress testing.
 
-class RandomPolicy : public InlinePolicy
+class RandomPolicy : public LegalPolicy
 {
 public:
 
@@ -134,7 +160,6 @@ public:
     // Policy observations
     void NoteSuccess() override;
     void NoteBool(InlineObservation obs, bool value) override;
-    void NoteFatal(InlineObservation obs) override;
     void NoteInt(InlineObservation obs, int value) override;
 
     // Policy determinations
@@ -147,12 +172,6 @@ public:
 
 private:
 
-    // Helper methods
-    void NoteInternal(InlineObservation obs);
-    void SetCandidate(InlineObservation obs);
-    void SetFailure(InlineObservation obs);
-    void SetNever(InlineObservation obs);
-
     // Data members
     Compiler*               m_RootCompiler;
     CLRRandom*              m_Random;