Merge pull request #7788 from AndyAyersMS/InlinePins

Inliner: support inlining of methods with pinned locals

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' ">