From a46475cdacca8beeb02193c2298f1df62328a317 Mon Sep 17 00:00:00 2001
From: Andy Ayers <andya@microsoft.com>
Date: Fri, 5 Jan 2018 16:41:02 -0800
Subject: [PATCH] JIT: fix issue with inline observations (#15713)

In DEBUG/CHECK builds the jit tries to keep track of failed inlines.

Because inlines can be rejected "early" (when the parent method
is being imported) as well as "late" (when their call site is encountered
by the inliner) there is a tracking mechanism to convey the early observations
that cause failures to be resurrected later on.

These observations sometimes didn't end up in the inline context, leading
to assertions when dumping methods.

Fix is to add a new way to propagate the earlier observation to the context
that bypasses some of the policy sanity checks and simply record the reason
that the inline failed.
---
 src/jit/flowgraph.cpp    | 18 ++----------------
 src/jit/inline.cpp       |  2 ++
 src/jit/inline.h         | 14 ++++++++++++++
 src/jit/inlinepolicy.cpp | 21 +++++++++++++++++++++
 src/jit/inlinepolicy.h   |  7 +++++++
 5 files changed, 46 insertions(+), 16 deletions(-)

diff --git a/src/jit/flowgraph.cpp b/src/jit/flowgraph.cpp
index 7dbf463..22fd0d3 100644
--- a/src/jit/flowgraph.cpp
+++ b/src/jit/flowgraph.cpp
@@ -22077,22 +22077,8 @@ void Compiler::fgNoteNonInlineCandidate(GenTreeStmt* stmt, GenTreeCall* call)
         currentObservation = priorObservation;
     }
 
-    // Would like to just call noteFatal here, since this
-    // observation blocked candidacy, but policy comes into play
-    // here too.  Also note there's no need to re-report these
-    // failures, since we reported them during the initial
-    // candidate scan.
-    InlineImpact impact = InlGetImpact(currentObservation);
-
-    if (impact == InlineImpact::FATAL)
-    {
-        inlineResult.NoteFatal(currentObservation);
-    }
-    else
-    {
-        inlineResult.Note(currentObservation);
-    }
-
+    // Propagate the prior failure observation to this result.
+    inlineResult.NotePriorFailure(currentObservation);
     inlineResult.SetReported();
 
     if (call->gtCallType == CT_USER_FUNC)
diff --git a/src/jit/inline.cpp b/src/jit/inline.cpp
index 2bff932..36a8802 100644
--- a/src/jit/inline.cpp
+++ b/src/jit/inline.cpp
@@ -1260,6 +1260,8 @@ InlineContext* InlineStrategy::NewFailure(GenTreeStmt* stmt, InlineResult* inlin
     failedContext->m_Callee      = inlineResult->GetCallee();
     failedContext->m_Success     = false;
 
+    assert(InlIsValidObservation(failedContext->m_Observation));
+
 #if defined(DEBUG) || defined(INLINE_DATA)
 
     // Update offset with more accurate info
diff --git a/src/jit/inline.h b/src/jit/inline.h
index fb1d7a2..3402c52 100644
--- a/src/jit/inline.h
+++ b/src/jit/inline.h
@@ -254,6 +254,9 @@ public:
 
 #if defined(DEBUG) || defined(INLINE_DATA)
 
+    // Record observation for prior failure
+    virtual void NotePriorFailure(InlineObservation obs) = 0;
+
     // Name of the policy
     virtual const char* GetName() const = 0;
     // Detailed data value dump
@@ -398,6 +401,17 @@ public:
         m_Policy->NoteInt(obs, value);
     }
 
+#if defined(DEBUG) || defined(INLINE_DATA)
+
+    // Record observation from an earlier failure.
+    void NotePriorFailure(InlineObservation obs)
+    {
+        m_Policy->NotePriorFailure(obs);
+        assert(IsFailure());
+    }
+
+#endif // defined(DEBUG) || defined(INLINE_DATA)
+
     // Determine if this inline is profitable
     void DetermineProfitability(CORINFO_METHOD_INFO* methodInfo)
     {
diff --git a/src/jit/inlinepolicy.cpp b/src/jit/inlinepolicy.cpp
index 2d4ebfd..959ef1b 100644
--- a/src/jit/inlinepolicy.cpp
+++ b/src/jit/inlinepolicy.cpp
@@ -104,6 +104,27 @@ void LegalPolicy::NoteFatal(InlineObservation obs)
     assert(InlDecisionIsFailure(m_Decision));
 }
 
+#if defined(DEBUG) || defined(INLINE_DATA)
+
+//------------------------------------------------------------------------
+// NotePriorFailure: record reason for earlier inline failure
+//
+// Arguments:
+//    obs      - the current obsevation
+//
+// Notes:
+//    Used to "resurrect" failure observations from the early inline
+//    screen when building the inline context tree. Only used during
+//    debug modes.
+
+void LegalPolicy::NotePriorFailure(InlineObservation obs)
+{
+    NoteInternal(obs);
+    assert(InlDecisionIsFailure(m_Decision));
+}
+
+#endif // defined(DEBUG) || defined(INLINE_DATA)
+
 //------------------------------------------------------------------------
 // NoteInternal: helper for handling an observation
 //
diff --git a/src/jit/inlinepolicy.h b/src/jit/inlinepolicy.h
index b1094bb..c7a8dc5 100644
--- a/src/jit/inlinepolicy.h
+++ b/src/jit/inlinepolicy.h
@@ -56,6 +56,13 @@ public:
     // Handle an observation that must cause inlining to fail.
     void NoteFatal(InlineObservation obs) override;
 
+#if defined(DEBUG) || defined(INLINE_DATA)
+
+    // Record observation for prior failure
+    void NotePriorFailure(InlineObservation obs) override;
+
+#endif // defined(DEBUG) || defined(INLINE_DATA)
+
 protected:
     // Helper methods
     void NoteInternal(InlineObservation obs);
-- 
2.7.4