InlineRefactoring: capturing bits of legacy policy

Rework the logic for force inline, basic block count, il size,
and maxstack so that the policy decides when these values
should inhibit inlining.

7 files changed, 103 insertions, 37 deletions

diff --git a/src/jit/compiler.cpp b/src/jit/compiler.cpp
index 0af9efcd26..5cca54840f 100644
--- a/src/jit/compiler.cpp
+++ b/src/jit/compiler.cpp
@@ -4962,13 +4962,16 @@ int           Compiler::compCompileHelper (CORINFO_MODULE_HANDLE            clas
         compGenTreeID = 0;
 #endif
 
-        if (compIsForInlining() &&
-            (fgBBcount > 5) &&
-            !forceInline)
+        if (compIsForInlining())
         {
-            compInlineResult->note(InlineObservation::CALLEE_TOO_MANY_BASIC_BLOCKS);
+            if (forceInline)
+            {
+                compInlineResult->noteCandidate(InlineObservation::CALLEE_IS_FORCE_INLINE);
+            }
+
+            compInlineResult->noteInt(InlineObservation::CALLEE_NUMBER_OF_BASIC_BLOCKS, fgBBcount);
 
-            if (compInlineResult->isFailure()) 
+            if (compInlineResult->isFailure())
             {
                 goto _Next;
             }
diff --git a/src/jit/compiler.h b/src/jit/compiler.h
index bb302ee623..0add1fd247 100644
--- a/src/jit/compiler.h
+++ b/src/jit/compiler.h
@@ -2509,6 +2509,9 @@ public :
 
     InlineInfo         * impInlineInfo;      
 
+    // Get the maximum IL size allowed for an inline
+    unsigned             getImpInlineSize() const { return impInlineSize; }
+
     // The Compiler* that is the root of the inlining tree of which "this" is a member.
     Compiler*            impInlineRoot();
 
@@ -2583,8 +2586,10 @@ protected :
     //-------------------- Stack manipulation ---------------------------------
 
     unsigned            impStkSize;   // Size of the full stack
-    StackEntry          impSmallStack[16];  // Use this array is possible
 
+#define SMALL_STACK_SIZE 16           // number of elements in impSmallStack
+
+    StackEntry          impSmallStack[SMALL_STACK_SIZE];  // Use this array if possible
 
     struct SavedStack                   // used to save/restore stack contents.
     {
@@ -3077,10 +3082,6 @@ private:
     static unsigned jitIciStmtIsTheLastInBB;
     static unsigned jitInlineeContainsOnlyOneBB;
 
-    #define             INLINER_FAILED                  "\nINLINER FAILED: "
-    #define             INLINER_WARNING                 "\nINLINER WARNING: "
-    #define             INLINER_INFO                    "\nINLINER INFO: "
-
 #endif // defined(DEBUG) || MEASURE_INLINING
 
 #ifdef DEBUG
diff --git a/src/jit/flowgraph.cpp b/src/jit/flowgraph.cpp
index 0dc9fe67bf..eca2e19189 100644
--- a/src/jit/flowgraph.cpp
+++ b/src/jit/flowgraph.cpp
@@ -4423,8 +4423,11 @@ DECODE_OPCODE:
 
             if (compIsForInlining())
             {
-                compInlineResult->noteFatal(InlineObservation::CALLEE_HAS_SWITCH);
-                return;
+                compInlineResult->note(InlineObservation::CALLEE_HAS_SWITCH);
+                if (compInlineResult->isFailure()) 
+                {
+                    return;
+                }
             }
 
             // Make sure we don't go past the end reading the number of cases
@@ -21919,7 +21922,7 @@ void       Compiler::fgInvokeInlineeCompiler(GenTreeCall*  call,
 
             JITLOG_THIS(pParam->pThis,
                         (LL_INFO100000,
-                         INLINER_INFO "inlineInfo.tokenLookupContextHandle for %s set to 0x%p:\n",
+                         "INLINER: inlineInfo.tokenLookupContextHandle for %s set to 0x%p:\n",
                          pParam->pThis->eeGetMethodFullName(pParam->fncHandle),
                          pParam->pThis->dspPtr(pParam->inlineInfo->tokenLookupContextHandle)));
 
diff --git a/src/jit/importer.cpp b/src/jit/importer.cpp
index e4e5e0c5f1..dc79b763a2 100644
--- a/src/jit/importer.cpp
+++ b/src/jit/importer.cpp
@@ -15818,6 +15818,11 @@ void Compiler::impCanInlineIL(CORINFO_METHOD_HANDLE    fncHandle,
         return;
     }
 
+    if (forceInline)
+    {
+        inlineResult->noteCandidate(InlineObservation::CALLEE_IS_FORCE_INLINE);
+    }
+
     // Reject if it has too many locals.
     // This is currently an implementation limit due to fixed-size arrays in the
     // inline info, rather than a performance heuristic.
@@ -15842,36 +15847,21 @@ void Compiler::impCanInlineIL(CORINFO_METHOD_HANDLE    fncHandle,
         return;
     }
 
-    if (forceInline) 
-    {
-        // This looks a bit redundant; it's because we haven't yet
-        // extracted policy from observation....
-        inlineResult->note(InlineObservation::CALLEE_HAS_FORCE_INLINE);
-        inlineResult->noteCandidate(InlineObservation::CALLEE_HAS_FORCE_INLINE);
-        return;
-    }       
- 
     // Reject large functions
 
     inlineResult->noteInt(InlineObservation::CALLEE_NUMBER_OF_IL_BYTES, codeSize);
 
-    if (codeSize > impInlineSize)
+    if (inlineResult->isFailure())
     {
-        inlineResult->note(InlineObservation::CALLEE_TOO_MUCH_IL);
-
-        if (inlineResult->isFailure())
-        {
-            return;
-        }
+        return;
     }
 
     // Make sure maxstack is not too big
 
     inlineResult->noteInt(InlineObservation::CALLEE_MAXSTACK, methInfo->maxStack);
 
-    if (methInfo->maxStack > sizeof(impSmallStack)/sizeof(impSmallStack[0]))
+    if (inlineResult->isFailure())
     {
-        inlineResult->noteFatal(InlineObservation::CALLEE_MAXSTACK_TOO_BIG);
         return;
     }
 
diff --git a/src/jit/inline.def b/src/jit/inline.def
index fa2ab24e8e..d602af8135 100644
--- a/src/jit/inline.def
+++ b/src/jit/inline.def
@@ -41,7 +41,6 @@ INLINE_OBSERVATION(HAS_NATIVE_VARARGS,        bool,   "native varargs",
 INLINE_OBSERVATION(HAS_NO_BODY,               bool,   "has no body",                   FATAL,       CALLEE)
 INLINE_OBSERVATION(HAS_NULL_FOR_LDELEM,       bool,   "has null pointer for ldelem",   FATAL,       CALLEE)
 INLINE_OBSERVATION(HAS_PINNED_LOCALS,         bool,   "has pinned locals",             FATAL,       CALLEE)
-INLINE_OBSERVATION(HAS_SWITCH,                bool,   "has switch",                    FATAL,       CALLEE)
 INLINE_OBSERVATION(IS_ARRAY_METHOD,           bool,   "is array method",               FATAL,       CALLEE)
 INLINE_OBSERVATION(IS_GENERIC_VIRTUAL,        bool,   "generic virtual",               FATAL,       CALLEE)
 INLINE_OBSERVATION(IS_JIT_NOINLINE,           bool,   "noinline per JitNoinline",      FATAL,       CALLEE)
@@ -66,6 +65,7 @@ INLINE_OBSERVATION(UNSUPPORTED_OPCODE,        bool,   "unsupported opcode",
 
 // ------ Callee Performance ------- 
 
+INLINE_OBSERVATION(HAS_SWITCH,                bool,   "has switch",                    PERFORMANCE, CALLEE)
 INLINE_OBSERVATION(LDFLD_STATIC_VALUECLASS,   bool,   "ldsfld of value class",         PERFORMANCE, CALLEE)
 INLINE_OBSERVATION(TOO_MANY_BASIC_BLOCKS,     bool,   "too many basic blocks",         PERFORMANCE, CALLEE)
 INLINE_OBSERVATION(TOO_MUCH_IL,               bool,   "too many il bytes",             PERFORMANCE, CALLEE)
@@ -75,10 +75,11 @@ INLINE_OBSERVATION(TOO_MUCH_IL,               bool,   "too many il bytes",
 INLINE_OBSERVATION(BELOW_ALWAYS_INLINE_SIZE,  bool,   "below ALWAYS_INLINE size",      INFORMATION, CALLEE)
 INLINE_OBSERVATION(CAN_INLINE_IL,             bool,   "IL passes basic checks",        INFORMATION, CALLEE)
 INLINE_OBSERVATION(CHECK_CAN_INLINE_IL,       bool,   "IL passes detailed checks",     INFORMATION, CALLEE)
-INLINE_OBSERVATION(HAS_FORCE_INLINE,          bool,   "aggressive inline attribute",   INFORMATION, CALLEE)
+INLINE_OBSERVATION(IS_FORCE_INLINE,           bool,   "aggressive inline attribute",   INFORMATION, CALLEE)
 INLINE_OBSERVATION(MAXSTACK,                  int,    "maxstack",                      INFORMATION, CALLEE)
 INLINE_OBSERVATION(NATIVE_SIZE_ESTIMATE,      double, "native size estimate",          INFORMATION, CALLEE)
 INLINE_OBSERVATION(NUMBER_OF_ARGUMENTS,       int,    "number of arguments",           INFORMATION, CALLEE)
+INLINE_OBSERVATION(NUMBER_OF_BASIC_BLOCKS,    int,    "number of basic blocks",        INFORMATION, CALLEE)
 INLINE_OBSERVATION(NUMBER_OF_IL_BYTES,        int,    "number of bytes of IL",         INFORMATION, CALLEE)
 INLINE_OBSERVATION(NUMBER_OF_LOCALS,          int,    "number of locals",              INFORMATION, CALLEE)
 
diff --git a/src/jit/inlinepolicy.cpp b/src/jit/inlinepolicy.cpp
index 829a724442..0be86bf10b 100644
--- a/src/jit/inlinepolicy.cpp
+++ b/src/jit/inlinepolicy.cpp
@@ -25,7 +25,7 @@
 InlinePolicy* InlinePolicy::getPolicy(Compiler* compiler)
 {
     // For now, always create a Legacy policy.
-    InlinePolicy* policy = new (compiler, CMK_Inlining) LegacyPolicy();
+    InlinePolicy* policy = new (compiler, CMK_Inlining) LegacyPolicy(compiler);
 
     return policy;
 }
@@ -44,6 +44,18 @@ void LegacyPolicy::noteCandidate(InlineObservation obs)
     InlineImpact impact = inlGetImpact(obs);
     assert(impact == InlineImpact::INFORMATION);
 
+    switch (obs)
+    {
+    case InlineObservation::CALLEE_IS_FORCE_INLINE:
+        {
+            inlIsForceInline = true;
+            break;
+        }
+
+    default:
+        break;
+    }
+
     // Update the status
     setCommon(InlineDecision::CANDIDATE, obs);
 }
@@ -71,6 +83,7 @@ void LegacyPolicy::note(InlineObservation obs)
     // As a safeguard, all fatal impact must be
     // reported via noteFatal.
     assert(impact != InlineImpact::FATAL);
+
     noteInternal(obs, impact);
 }
 
@@ -101,8 +114,53 @@ void LegacyPolicy::noteFatal(InlineObservation obs)
 
 void LegacyPolicy::noteInt(InlineObservation obs, int value)
 {
-    (void) value;
-    note(obs);
+    switch (obs)
+    {
+    case InlineObservation::CALLEE_MAXSTACK:
+        {
+            unsigned calleeMaxStack = static_cast<unsigned>(value);
+
+            if (calleeMaxStack > SMALL_STACK_SIZE)
+            {
+                setNever(InlineObservation::CALLEE_MAXSTACK_TOO_BIG);
+            }
+
+            break;
+        }
+
+    case InlineObservation::CALLEE_NUMBER_OF_BASIC_BLOCKS:
+        {
+            assert(value != 0);
+
+            unsigned basicBlockCount = static_cast<unsigned>(value);
+
+            if (!inlIsForceInline && (basicBlockCount > MAX_BASIC_BLOCKS))
+            {
+                setNever(InlineObservation::CALLEE_TOO_MANY_BASIC_BLOCKS);
+            }
+
+            break;
+        }
+
+    case InlineObservation::CALLEE_NUMBER_OF_IL_BYTES:
+        {
+            assert(value != 0);
+
+            unsigned ilByteSize = static_cast<unsigned>(value);
+
+            if (ilByteSize > inlCompiler->getImpInlineSize())
+            {
+                setNever(InlineObservation::CALLEE_TOO_MUCH_IL);
+            }
+
+            break;
+        }
+
+    default:
+        // Ignore all other information
+        note(obs);
+        break;
+    }
 }
 
 //------------------------------------------------------------------------
diff --git a/src/jit/inlinepolicy.h b/src/jit/inlinepolicy.h
index 3f883b2bec..7a4c6fb6bd 100644
--- a/src/jit/inlinepolicy.h
+++ b/src/jit/inlinepolicy.h
@@ -24,8 +24,11 @@ class LegacyPolicy : public InlinePolicy
 {
 public:
 
-    LegacyPolicy()
+    // Construct a LegacyPolicy
+    LegacyPolicy(Compiler* compiler)
         : InlinePolicy()
+        , inlCompiler(compiler)
+        , inlIsForceInline(false)
     {
         // empty
     }
@@ -52,6 +55,13 @@ private:
     void setFailure(InlineObservation obs);
     void setNever(InlineObservation obs);
     void setCommon(InlineDecision decision, InlineObservation obs);
+
+    // Constants
+    const unsigned MAX_BASIC_BLOCKS = 5;
+
+    // Data members
+    Compiler* inlCompiler;
+    bool      inlIsForceInline;
 };
 
 //