From c60e68e51a8a09886df5e085986dae76cd74f974 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Fri, 21 Oct 2016 18:46:50 -0700 Subject: [PATCH] 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. --- src/jit/compiler.h | 1 + src/jit/flowgraph.cpp | 123 ++++++++++++++++-- src/jit/importer.cpp | 36 ++++- src/jit/inline.def | 6 +- src/jit/inline.h | 2 + src/jit/inlinepolicy.cpp | 17 +++ src/jit/inlinepolicy.h | 2 + .../FixedAddressValueType.csproj | 1 + 8 files changed, 170 insertions(+), 18 deletions(-) diff --git a/src/jit/compiler.h b/src/jit/compiler.h index 8f36820b9957..cd5da2503bbe 100644 --- a/src/jit/compiler.h +++ b/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); diff --git a/src/jit/flowgraph.cpp b/src/jit/flowgraph.cpp index d9c1f21833e3..e362fb3d4d36 100644 --- a/src/jit/flowgraph.cpp +++ b/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*/ diff --git a/src/jit/importer.cpp b/src/jit/importer.cpp index 5709818ba993..abbe4631de49 100644 --- a/src/jit/importer.cpp +++ b/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)) diff --git a/src/jit/inline.def b/src/jit/inline.def index d0e013700eed..ff0b21100ef1 100644 --- a/src/jit/inline.def +++ b/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) diff --git a/src/jit/inline.h b/src/jit/inline.h index 9d625c729ce4..2bc483c4b7e5 100644 --- a/src/jit/inline.h +++ b/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 diff --git a/src/jit/inlinepolicy.cpp b/src/jit/inlinepolicy.cpp index ff584d0e4666..c67b32c33c06 100644 --- a/src/jit/inlinepolicy.cpp +++ b/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); diff --git a/src/jit/inlinepolicy.h b/src/jit/inlinepolicy.h index ba79bfa779c7..4b1e55e082f6 100644 --- a/src/jit/inlinepolicy.h +++ b/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 diff --git a/tests/src/baseservices/compilerservices/FixedAddressValueType/FixedAddressValueType.csproj b/tests/src/baseservices/compilerservices/FixedAddressValueType/FixedAddressValueType.csproj index 72ba0b9741c5..19a779194fe4 100644 --- a/tests/src/baseservices/compilerservices/FixedAddressValueType/FixedAddressValueType.csproj +++ b/tests/src/baseservices/compilerservices/FixedAddressValueType/FixedAddressValueType.csproj @@ -16,6 +16,7 @@ true BuildAndRun true + true