Inliner: support inlining of methods with pinned locals

The inliner now can inline a subset of methods with pinned locals --
namely those where the language compiler (eg CSC) can determine that a
try/finally is not necessary to ensure unpinning, and where the jit
likewise an determines that a try/finally is likewise not needed in
the root method.

Generally speaking this allows inlining in cases where the inline method
is fairly simple and does not any contain exception handling, and the
call site is not within a try region.

When inlining methods that have pinned locals and also return a value,
the jit ensures that the return value is spilled to a temp so that the
unpins can be placed just after the inlined method body and won't alter
the return value.

Mark FixedAddressValueType as GCStressIncompatible since the "unfixed"
class static may get re-allocated at the same address. This seems to
happen even without these changes but happens much more frequently with
them.

Closes #7774.

Author: AndyAyersMS

src/jit/compiler.h

@@ -4664,6 +4664,7 @@ class Compiler
     void fgInvokeInlineeCompiler(GenTreeCall* call, InlineResult* result);
     void fgInsertInlineeBlocks(InlineInfo* pInlineInfo);
     GenTreePtr fgInlinePrependStatements(InlineInfo* inlineInfo);
+    void fgInlineAppendStatements(InlineInfo* inlineInfo, BasicBlock* block, GenTreePtr stmt);
 
 #if FEATURE_MULTIREG_RET
     GenTreePtr fgGetStructAsStructPtr(GenTreePtr tree);

src/jit/flowgraph.cpp

@@ -4777,6 +4777,12 @@ void Compiler::fgFindJumpTargets(const BYTE* codeAddr, IL_OFFSET codeSize, BYTE*
             }
         }
 
+        // Determine if call site is within a try.
+        if (isInlining && impInlineInfo->iciBlock->hasTryIndex())
+        {
+            compInlineResult->Note(InlineObservation::CALLSITE_IN_TRY_REGION);
+        }
+
         // If the inline is viable and discretionary, do the
         // profitability screening.
         if (compInlineResult->IsDiscretionaryCandidate())
@@ -5774,12 +5780,14 @@ void Compiler::fgFindBasicBlocks()
         compHndBBtabCount    = impInlineInfo->InlinerCompiler->compHndBBtabCount;
         info.compXcptnsCount = impInlineInfo->InlinerCompiler->info.compXcptnsCount;
 
-        if (info.compRetNativeType != TYP_VOID && retBlocks > 1)
+        // Use a spill temp for the return value if there are multiple return blocks.
+        if ((info.compRetNativeType != TYP_VOID) && (retBlocks > 1))
         {
             // The lifetime of this var might expand multiple BBs. So it is a long lifetime compiler temp.
             lvaInlineeReturnSpillTemp = lvaGrabTemp(false DEBUGARG("Inline candidate multiple BBJ_RETURN spill temp"));
             lvaTable[lvaInlineeReturnSpillTemp].lvType = info.compRetNativeType;
         }
+
         return;
     }
 
@@ -21696,11 +21704,8 @@ void Compiler::fgInsertInlineeBlocks(InlineInfo* pInlineInfo)
         }
     }
 
-    //
     // Prepend statements.
-    //
-    GenTreePtr stmtAfter;
-    stmtAfter = fgInlinePrependStatements(pInlineInfo);
+    GenTreePtr stmtAfter = fgInlinePrependStatements(pInlineInfo);
 
 #ifdef DEBUG
     if (verbose)
@@ -21710,6 +21715,9 @@ void Compiler::fgInsertInlineeBlocks(InlineInfo* pInlineInfo)
     }
 #endif // DEBUG
 
+    BasicBlock* topBlock = iciBlock;
+    BasicBlock* bottomBlock = nullptr;
+
     if (InlineeCompiler->fgBBcount == 1)
     {
         // When fgBBCount is 1 we will always have a non-NULL fgFirstBB
@@ -21734,6 +21742,7 @@ void Compiler::fgInsertInlineeBlocks(InlineInfo* pInlineInfo)
                 noway_assert((inlineeBlockFlags & BBF_KEEP_BBJ_ALWAYS) == 0);
                 iciBlock->bbFlags |= inlineeBlockFlags;
             }
+
 #ifdef DEBUG
             if (verbose)
             {
@@ -21756,6 +21765,10 @@ void Compiler::fgInsertInlineeBlocks(InlineInfo* pInlineInfo)
                 }
             }
 #endif // DEBUG
+
+            // Append statements to unpin, if necessary.
+            fgInlineAppendStatements(pInlineInfo, iciBlock, stmtAfter);
+
             goto _Done;
         }
     }
@@ -21764,11 +21777,6 @@ void Compiler::fgInsertInlineeBlocks(InlineInfo* pInlineInfo)
     // ======= Inserting inlinee's basic blocks ===============
     //
 
-    BasicBlock* topBlock;
-    BasicBlock* bottomBlock;
-
-    topBlock = iciBlock;
-
     bottomBlock = fgNewBBafter(topBlock->bbJumpKind, topBlock, true);
     bottomBlock->bbRefs = 1;
     bottomBlock->bbJumpDest = topBlock->bbJumpDest;
@@ -21929,6 +21937,9 @@ void Compiler::fgInsertInlineeBlocks(InlineInfo* pInlineInfo)
     //
     fgBBcount += InlineeCompiler->fgBBcount;
 
+    // Append statements to unpin if necessary.
+    fgInlineAppendStatements(pInlineInfo, bottomBlock, nullptr);
+
 #ifdef DEBUG
     if (verbose)
     {
@@ -21994,8 +22005,15 @@ void Compiler::fgInsertInlineeBlocks(InlineInfo* pInlineInfo)
     iciStmt->gtStmt.gtStmtExpr = gtNewNothingNode();
 }
 
-// Prepend the statements that are needed before the inlined call.
-// Return the last statement that is prepended.
+//------------------------------------------------------------------------
+// fgInlinePrependStatements: Prepend statements that are needed
+// before the inlined call. 
+//
+// Arguments:
+//    inlineInfo - information about the inline
+//
+// Return Value:
+//    The last statement that was prepended.
 
 GenTreePtr      Compiler::fgInlinePrependStatements(InlineInfo* inlineInfo)
 {
@@ -22276,6 +22294,87 @@ GenTreePtr      Compiler::fgInlinePrependStatements(InlineInfo* inlineInfo)
     return afterStmt;
 }
 
+//------------------------------------------------------------------------
+// fgInlineAppendStatements: Append statements that are needed
+// after the inlined call. 
+//
+// Arguments:
+//    inlineInfo - information about the inline
+//    block      - basic block for the new statements
+//    stmtAfter  - (optional) insertion point for mid-block cases
+
+void Compiler::fgInlineAppendStatements(InlineInfo* inlineInfo, BasicBlock* block, GenTreePtr stmtAfter)
+{
+    // Null out any inline pinned locals
+    if (!inlineInfo->hasPinnedLocals)
+    {
+        // No pins, nothing to do
+        return;
+    }
+
+    JITDUMP("Unpin inlinee locals:\n");
+
+    GenTreePtr callStmt  = inlineInfo->iciStmt;
+    IL_OFFSETX callILOffset = callStmt->gtStmt.gtStmtILoffsx;
+    CORINFO_METHOD_INFO* InlineeMethodInfo = InlineeCompiler->info.compMethodInfo;
+    unsigned lclCnt = InlineeMethodInfo->locals.numArgs;
+    InlLclVarInfo*    lclVarInfo = inlineInfo->lclVarInfo;
+
+    noway_assert(callStmt->gtOper == GT_STMT);
+
+    for (unsigned lclNum = 0; lclNum < lclCnt; lclNum++)
+    {
+        unsigned tmpNum = inlineInfo->lclTmpNum[lclNum];
+
+        // Is the local used at all?
+        if (tmpNum == BAD_VAR_NUM)
+        {
+            // Nope, nothing to unpin.
+            continue;
+        }
+
+        // Is the local pinned?
+        if (!lvaTable[tmpNum].lvPinned)
+        {
+            // Nope, nothing to unpin.
+            continue;
+        }
+
+        // Does the local we're about to unpin appear in the return
+        // expression?  If so we somehow messed up and didn't properly
+        // spill the return value. See impInlineFetchLocal.
+        GenTreePtr retExpr = inlineInfo->retExpr;
+        if (retExpr != nullptr)
+        {
+            const bool interferesWithReturn = gtHasRef(inlineInfo->retExpr, tmpNum, false);
+            noway_assert(!interferesWithReturn);
+        }
+
+        // Emit the unpin, by assigning null to the local.
+        var_types lclTyp = (var_types)lvaTable[tmpNum].lvType;
+        noway_assert(lclTyp == lclVarInfo[lclNum + inlineInfo->argCnt].lclTypeInfo);
+        noway_assert(!varTypeIsStruct(lclTyp));
+        GenTreePtr unpinExpr = gtNewTempAssign(tmpNum, gtNewZeroConNode(genActualType(lclTyp)));
+        GenTreePtr unpinStmt = gtNewStmt(unpinExpr, callILOffset);
+
+        if (stmtAfter == nullptr)
+        {
+            stmtAfter = fgInsertStmtAtBeg(block, unpinStmt);
+        }
+        else
+        {
+            stmtAfter = fgInsertStmtAfter(block, stmtAfter, unpinStmt);
+        }
+
+#ifdef DEBUG
+        if (verbose)
+        {
+            gtDispTree(unpinStmt);
+        }
+#endif // DEBUG
+
+    }
+}
 
 /*****************************************************************************/
 /*static*/

src/jit/importer.cpp

@@ -14911,7 +14911,8 @@ bool Compiler::impReturnInstruction(BasicBlock* block, int prefixFlags, OPCODE&
 
                 if (lvaInlineeReturnSpillTemp != BAD_VAR_NUM)
                 {
-                    assert(info.compRetNativeType != TYP_VOID && fgMoreThanOneReturnBlock());
+                    assert(info.compRetNativeType != TYP_VOID &&
+                           (fgMoreThanOneReturnBlock() || impInlineInfo->hasPinnedLocals));
 
                     // This is a bit of a workaround...
                     // If we are inlining a call that returns a struct, where the actual "native" return type is
@@ -15002,7 +15003,7 @@ bool Compiler::impReturnInstruction(BasicBlock* block, int prefixFlags, OPCODE&
                     // in this case we have to insert multiple struct copies to the temp
                     // and the retexpr is just the temp.
                     assert(info.compRetNativeType != TYP_VOID);
-                    assert(fgMoreThanOneReturnBlock());
+                    assert(fgMoreThanOneReturnBlock() || impInlineInfo->hasPinnedLocals);
 
                     impAssignTempGen(lvaInlineeReturnSpillTemp, op2, se.seTypeInfo.GetClassHandle(),
                                      (unsigned)CHECK_SPILL_ALL);
@@ -17429,12 +17430,17 @@ void Compiler::impInlineInitVars(InlineInfo* pInlineInfo)
         var_types type = (var_types)eeGetArgType(localsSig, &methInfo->locals, &isPinned);
 
         lclVarInfo[i + argCnt].lclHasLdlocaOp = false;
+        lclVarInfo[i + argCnt].lclIsPinned    = isPinned;
         lclVarInfo[i + argCnt].lclTypeInfo    = type;
 
         if (isPinned)
         {
-            inlineResult->NoteFatal(InlineObservation::CALLEE_HAS_PINNED_LOCALS);
-            return;
+            // Pinned locals may cause inlines to fail.
+            inlineResult->Note(InlineObservation::CALLEE_HAS_PINNED_LOCALS);
+            if (inlineResult->IsFailure())
+            {
+                return;
+            }
         }
 
         lclVarInfo[i + argCnt].lclVerTypeInfo = verParseArgSigToTypeInfo(&methInfo->locals, localsSig);
@@ -17511,6 +17517,28 @@ unsigned Compiler::impInlineFetchLocal(unsigned lclNum DEBUGARG(const char* reas
             lvaTable[tmpNum].lvHasLdAddrOp = 1;
         }
 
+        if (impInlineInfo->lclVarInfo[lclNum + impInlineInfo->argCnt].lclIsPinned)
+        {
+            lvaTable[tmpNum].lvPinned = 1;
+
+            if (!impInlineInfo->hasPinnedLocals)
+            {
+                // If the inlinee returns a value, use a spill temp
+                // for the return value to ensure that even in case
+                // where the return expression refers to one of the
+                // pinned locals, we can unpin the local right after
+                // the inlined method body.
+                if ((info.compRetNativeType != TYP_VOID) && (lvaInlineeReturnSpillTemp == BAD_VAR_NUM))
+                {
+                    lvaInlineeReturnSpillTemp =
+                        lvaGrabTemp(false DEBUGARG("Inline candidate pinned local return spill temp"));
+                    lvaTable[lvaInlineeReturnSpillTemp].lvType = info.compRetNativeType;
+                }
+            }
+
+            impInlineInfo->hasPinnedLocals = true;
+        }
+
         if (impInlineInfo->lclVarInfo[lclNum + impInlineInfo->argCnt].lclVerTypeInfo.IsStruct())
         {
             if (varTypeIsStruct(lclTyp))

src/jit/inline.def

@@ -40,7 +40,6 @@ INLINE_OBSERVATION(HAS_MANAGED_VARARGS,       bool,   "managed varargs",
 INLINE_OBSERVATION(HAS_NATIVE_VARARGS,        bool,   "native varargs",                FATAL,       CALLEE)
 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(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)
@@ -79,6 +78,7 @@ INLINE_OBSERVATION(CLASS_PROMOTABLE,          bool,   "promotable value class",
 INLINE_OBSERVATION(DOES_NOT_RETURN,           bool,   "does not return",               INFORMATION, CALLEE)
 INLINE_OBSERVATION(END_OPCODE_SCAN,           bool,   "done looking at opcodes",       INFORMATION, CALLEE)
 INLINE_OBSERVATION(HAS_GC_STRUCT,             bool,   "has gc field in struct local",  INFORMATION, CALLEE)
+INLINE_OBSERVATION(HAS_PINNED_LOCALS,         bool,   "has pinned locals",             INFORMATION, CALLEE)
 INLINE_OBSERVATION(HAS_SIMD,                  bool,   "has SIMD arg, local, or ret",   INFORMATION, CALLEE)
 INLINE_OBSERVATION(HAS_SWITCH,                bool,   "has switch",                    INFORMATION, CALLEE)
 INLINE_OBSERVATION(IL_CODE_SIZE,              int,    "number of bytes of IL",         INFORMATION, CALLEE)
@@ -139,7 +139,7 @@ INLINE_OBSERVATION(IS_TOO_DEEP,               bool,   "too deep",
 INLINE_OBSERVATION(IS_VIRTUAL,                bool,   "virtual",                       FATAL,       CALLSITE)
 INLINE_OBSERVATION(IS_VM_NOINLINE,            bool,   "noinline per VM",               FATAL,       CALLSITE)
 INLINE_OBSERVATION(IS_WITHIN_CATCH,           bool,   "within catch region",           FATAL,       CALLSITE)
-INLINE_OBSERVATION(IS_WITHIN_FILTER,          bool,   "within filterregion",           FATAL,       CALLSITE)
+INLINE_OBSERVATION(IS_WITHIN_FILTER,          bool,   "within filter region",          FATAL,       CALLSITE)
 INLINE_OBSERVATION(LDARGA_NOT_LOCAL_VAR,      bool,   "ldarga not on local var",       FATAL,       CALLSITE)
 INLINE_OBSERVATION(LDFLD_NEEDS_HELPER,        bool,   "ldfld needs helper",            FATAL,       CALLSITE)
 INLINE_OBSERVATION(LDVIRTFN_ON_NON_VIRTUAL,   bool,   "ldvirtfn on non-virtual",       FATAL,       CALLSITE)
@@ -148,6 +148,7 @@ INLINE_OBSERVATION(NOT_CANDIDATE,             bool,   "not inline candidate",
 INLINE_OBSERVATION(NOT_PROFITABLE_INLINE,     bool,   "unprofitable inline",           FATAL,       CALLSITE)
 INLINE_OBSERVATION(OVER_BUDGET,               bool,   "inline exceeds budget",         FATAL,       CALLSITE)
 INLINE_OBSERVATION(OVER_INLINE_LIMIT,         bool,   "limited by JitInlineLimit",     FATAL,       CALLSITE)
+INLINE_OBSERVATION(PIN_IN_TRY_REGION,         bool,   "within try region, pinned",     FATAL,       CALLSITE)
 INLINE_OBSERVATION(RANDOM_REJECT,             bool,   "random reject",                 FATAL,       CALLSITE)
 INLINE_OBSERVATION(REQUIRES_SAME_THIS,        bool,   "requires same this",            FATAL,       CALLSITE)
 INLINE_OBSERVATION(RETURN_TYPE_MISMATCH,      bool,   "return type mismatch",          FATAL,       CALLSITE)
@@ -163,6 +164,7 @@ INLINE_OBSERVATION(RARE_GC_STRUCT,            bool,   "rarely called, has gc str
 INLINE_OBSERVATION(CONSTANT_ARG_FEEDS_TEST,   bool,   "constant argument feeds test",  INFORMATION, CALLSITE)
 INLINE_OBSERVATION(DEPTH,                     int,    "depth",                         INFORMATION, CALLSITE)
 INLINE_OBSERVATION(FREQUENCY,                 int,    "rough call site frequency",     INFORMATION, CALLSITE)
+INLINE_OBSERVATION(IN_TRY_REGION,             bool,   "call site in try region",       INFORMATION, CALLSITE)
 INLINE_OBSERVATION(IS_PROFITABLE_INLINE,      bool,   "profitable inline",             INFORMATION, CALLSITE)
 INLINE_OBSERVATION(IS_SAME_THIS,              bool,   "same this as root caller",      INFORMATION, CALLSITE)
 INLINE_OBSERVATION(IS_SIZE_DECREASING_INLINE, bool,   "size decreasing inline",        INFORMATION, CALLSITE)

src/jit/inline.h

@@ -537,6 +537,7 @@ struct InlLclVarInfo
     var_types lclTypeInfo;
     typeInfo  lclVerTypeInfo;
     bool      lclHasLdlocaOp; // Is there LDLOCA(s) operation on this argument?
+    bool      lclIsPinned;
 };
 
 // InlineInfo provides detailed information about a particular inline candidate.
@@ -563,6 +564,7 @@ struct InlineInfo
     InlLclVarInfo lclVarInfo[MAX_INL_LCLS + MAX_INL_ARGS + 1]; // type information from local sig
 
     bool thisDereferencedFirst;
+    bool hasPinnedLocals;
 #ifdef FEATURE_SIMD
     bool hasSIMDTypeArgLocalOrReturn;
 #endif // FEATURE_SIMD

src/jit/inlinepolicy.cpp

@@ -383,6 +383,12 @@ void LegacyPolicy::NoteBool(InlineObservation obs, bool value)
                 break;
             }
 
+            case InlineObservation::CALLEE_HAS_PINNED_LOCALS:
+                // The legacy policy is to never inline methods with
+                // pinned locals.
+                SetNever(obs);
+                break;
+
             default:
                 // Ignore the remainder for now
                 break;
@@ -883,6 +889,17 @@ void EnhancedLegacyPolicy::NoteBool(InlineObservation obs, bool value)
             }
             break;
 
+        case InlineObservation::CALLEE_HAS_PINNED_LOCALS:
+            if (m_CallsiteIsInTryRegion)
+            {
+                // Inlining a method with pinned locals in a try
+                // region requires wrapping the inline body in a
+                // try/finally to ensure unpinning. Bail instead.
+                SetFailure(InlineObservation::CALLSITE_PIN_IN_TRY_REGION);
+                return;
+            }
+            break;
+
         default:
             // Pass all other information to the legacy policy
             LegacyPolicy::NoteBool(obs, value);

src/jit/inlinepolicy.h

@@ -98,6 +98,7 @@ class LegacyPolicy : public LegalPolicy
         , m_HasSimd(false)
         , m_LooksLikeWrapperMethod(false)
         , m_MethodIsMostlyLoadStore(false)
+        , m_CallsiteIsInTryRegion(false)
     {
         // empty
     }
@@ -165,6 +166,7 @@ class LegacyPolicy : public LegalPolicy
     bool                    m_HasSimd : 1;
     bool                    m_LooksLikeWrapperMethod : 1;
     bool                    m_MethodIsMostlyLoadStore : 1;
+    bool                    m_CallsiteIsInTryRegion : 1;
 };
 
 // EnhancedLegacyPolicy extends the legacy policy by rejecting

tests/src/baseservices/compilerservices/FixedAddressValueType/FixedAddressValueType.csproj

@@ -16,6 +16,7 @@
     <ReferenceLocalMscorlib>true</ReferenceLocalMscorlib>
     <CLRTestKind>BuildAndRun</CLRTestKind>
     <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
+    <GCStressIncompatible>true</GCStressIncompatible>
   </PropertyGroup>
   <!-- Default configurations to help VS understand the configurations -->
   <PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Debug|AnyCPU' ">