From 5f164ff93c31162f21a6441d524b9361e48f99ee Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 18 Oct 2022 19:11:17 +0200 Subject: [PATCH 1/6] JIT: Clean up inliner substitution * Stop using GenTree::ReplaceWith * GenTreeRetExpr now directly contains its substitution members, which is a tree either from the inline candidate, a local (due to spill temps) or the call itself (due to inliner failure). In the former case, it also contains the BasicBlock the child tree comes from to ensure that its flags can be propagated once and for all during substitution. * Make various members strongly typed, e.g. GenTreeRetExpr::gtInlineCandidate is now a GenTreeCall*, InlineCandidateInfo::retExpr is now a GenTreeRetExpr*. This is possible since we are no longer using GenTree::ReplaceWith. --- src/coreclr/jit/compiler.h | 2 +- src/coreclr/jit/fginline.cpp | 84 +++++++-------------- src/coreclr/jit/gentree.cpp | 14 ++-- src/coreclr/jit/gentree.h | 17 +++-- src/coreclr/jit/importer.cpp | 36 ++++----- src/coreclr/jit/importercalls.cpp | 2 +- src/coreclr/jit/indirectcalltransformer.cpp | 34 ++++----- src/coreclr/jit/inline.h | 23 +++--- src/coreclr/jit/morph.cpp | 21 ++---- 9 files changed, 97 insertions(+), 136 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index dc3e3c4171e798..dc40710409dd88 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -2583,7 +2583,7 @@ class Compiler GenTree* gtNewMustThrowException(unsigned helper, var_types type, CORINFO_CLASS_HANDLE clsHnd); GenTreeLclFld* gtNewLclFldNode(unsigned lnum, var_types type, unsigned offset); - GenTree* gtNewInlineCandidateReturnExpr(GenTree* inlineCandidate, var_types type, BasicBlockFlags bbFlags); + GenTreeRetExpr* gtNewInlineCandidateReturnExpr(GenTreeCall* inlineCandidate, var_types type); GenTreeField* gtNewFieldRef(var_types type, CORINFO_FIELD_HANDLE fldHnd, GenTree* obj = nullptr, DWORD offset = 0); diff --git a/src/coreclr/jit/fginline.cpp b/src/coreclr/jit/fginline.cpp index 9131c7eb6f3558..0f9e8c3d184cfd 100644 --- a/src/coreclr/jit/fginline.cpp +++ b/src/coreclr/jit/fginline.cpp @@ -100,7 +100,7 @@ class SubstitutePlaceholdersAndDevirtualizeWalker : public GenTreeVisitorOperIs(GT_RET_EXPR)) { - UpdateInlineReturnExpressionPlaceHolder(tree, user); + UpdateInlineReturnExpressionPlaceHolder(use, user); } #if FEATURE_MULTIREG_RET @@ -121,9 +121,10 @@ class SubstitutePlaceholdersAndDevirtualizeWalker : public GenTreeVisitorgtEffectiveVal(/*commaOnly*/ true); - noway_assert(!varTypeIsStruct(effectiveValue) || (effectiveValue->OperGet() != GT_RET_EXPR) || - !m_compiler->IsMultiRegReturnedType(effectiveValue->AsRetExpr()->gtRetClsHnd, - CorInfoCallConvExtension::Managed)); + noway_assert( + !varTypeIsStruct(effectiveValue) || (effectiveValue->OperGet() != GT_RET_EXPR) || + !m_compiler->IsMultiRegReturnedType(effectiveValue->AsRetExpr()->gtInlineCandidate->gtRetClsHnd, + CorInfoCallConvExtension::Managed)); } } @@ -144,8 +145,8 @@ class SubstitutePlaceholdersAndDevirtualizeWalker : public GenTreeVisitorOperIs(GT_RET_EXPR)) + while ((*use)->OperIs(GT_RET_EXPR)) { + GenTree* tree = *use; // We are going to copy the tree from the inlinee, // so record the handle now. // if (varTypeIsStruct(tree)) { - retClsHnd = tree->AsRetExpr()->gtRetClsHnd; + retClsHnd = tree->AsRetExpr()->gtInlineCandidate->gtRetClsHnd; } // Skip through chains of GT_RET_EXPRs (say from nested inlines) // to the actual tree to use. // - BasicBlockFlags bbFlags; - GenTree* inlineCandidate = tree; + BasicBlock* inlineeBB; + GenTree* inlineCandidate = tree; do { GenTreeRetExpr* retExpr = inlineCandidate->AsRetExpr(); - inlineCandidate = retExpr->gtInlineCandidate; - bbFlags = retExpr->bbFlags; + inlineCandidate = retExpr->gtSubstExpr; + inlineeBB = retExpr->gtSubstBB; } while (inlineCandidate->OperIs(GT_RET_EXPR)); // We might as well try and fold the return value. Eg returns of @@ -235,9 +237,13 @@ class SubstitutePlaceholdersAndDevirtualizeWalker : public GenTreeVisitorReplaceWith(inlineCandidate, m_compiler); + *use = inlineCandidate; m_madeChanges = true; - m_compiler->compCurBB->bbFlags |= (bbFlags & BBF_SPLIT_GAINED); + + if (inlineeBB != nullptr) + { + m_compiler->compCurBB->bbFlags |= (inlineeBB->bbFlags & BBF_SPLIT_GAINED); + } #ifdef DEBUG if (m_compiler->verbose) @@ -281,7 +287,7 @@ class SubstitutePlaceholdersAndDevirtualizeWalker : public GenTreeVisitorReplaceWith(AssignStructInlineeToVar(tree, retClsHnd), m_compiler); + *use = AssignStructInlineeToVar(tree, retClsHnd); } m_madeChanges = true; } @@ -853,8 +859,6 @@ void Compiler::fgInvokeInlineeCompiler(GenTreeCall* call, InlineResult* inlineRe inlineInfo.iciStmt = fgMorphStmt; inlineInfo.iciBlock = compCurBB; inlineInfo.thisDereferencedFirst = false; - inlineInfo.retExpr = nullptr; - inlineInfo.retBB = nullptr; inlineInfo.retExprClassHnd = nullptr; inlineInfo.retExprClassHndIsExact = false; inlineInfo.inlineResult = inlineResult; @@ -1006,12 +1010,12 @@ void Compiler::fgInvokeInlineeCompiler(GenTreeCall* call, InlineResult* inlineRe } #endif // DEBUG - // If there is non-NULL return, but we haven't set the pInlineInfo->retExpr, + // If there is non-NULL return, but we haven't set the substExpr, // That means we haven't imported any BB that contains CEE_RET opcode. // (This could happen for example for a BBJ_THROW block fall through a BBJ_RETURN block which // causes the BBJ_RETURN block not to be imported at all.) // Fail the inlining attempt - if (inlineCandidateInfo->fncRetType != TYP_VOID && inlineInfo.retExpr == nullptr) + if ((inlineCandidateInfo->fncRetType != TYP_VOID) && (inlineCandidateInfo->retExpr->gtSubstExpr == nullptr)) { #ifdef DEBUG if (verbose) @@ -1386,40 +1390,6 @@ void Compiler::fgInsertInlineeBlocks(InlineInfo* pInlineInfo) lvaSetVarDoNotEnregister(dummy DEBUGARG(DoNotEnregisterReason::VMNeedsStackAddr)); } - // If there is non-NULL return, replace the GT_CALL with its return value expression, - // so later it will be picked up by the GT_RET_EXPR node. - if ((pInlineInfo->inlineCandidateInfo->fncRetType != TYP_VOID) || (iciCall->gtReturnType == TYP_STRUCT)) - { - noway_assert(pInlineInfo->retExpr); -#ifdef DEBUG - if (verbose) - { - printf("\nReturn expression for call at "); - printTreeID(iciCall); - printf(" is\n"); - gtDispTree(pInlineInfo->retExpr); - } -#endif // DEBUG - - // Replace the call with the return expression. Note that iciCall won't be part of the IR - // but may still be referenced from a GT_RET_EXPR node. We will replace GT_RET_EXPR node - // in UpdateInlineReturnExpressionPlaceHolder. At that time we will also update the flags - // on the basic block of GT_RET_EXPR node. - if (iciCall->gtInlineCandidateInfo->retExpr->OperGet() == GT_RET_EXPR) - { - // Save the basic block flags from the retExpr basic block. - iciCall->gtInlineCandidateInfo->retExpr->AsRetExpr()->bbFlags = pInlineInfo->retBB->bbFlags; - } - - if (bottomBlock != nullptr) - { - // We've split the iciblock into two and the RET_EXPR was possibly moved to the bottomBlock - // so let's update its flags with retBB's ones - bottomBlock->bbFlags |= pInlineInfo->retBB->bbFlags & BBF_COMPACT_UPD; - } - iciCall->ReplaceWith(pInlineInfo->retExpr, this); - } - // // Detach the GT_CALL node from the original statement by hanging a "nothing" node under it, // so that fgMorphStmts can remove the statement once we return from here. @@ -1839,6 +1809,7 @@ void Compiler::fgInlineAppendStatements(InlineInfo* inlineInfo, BasicBlock* bloc InlLclVarInfo* lclVarInfo = inlineInfo->lclVarInfo; unsigned gcRefLclCnt = inlineInfo->numberOfGcRefLocals; const unsigned argCnt = inlineInfo->argCnt; + InlineCandidateInfo* inlCandInfo = inlineInfo->inlineCandidateInfo; for (unsigned lclNum = 0; lclNum < lclCnt; lclNum++) { @@ -1873,10 +1844,9 @@ void Compiler::fgInlineAppendStatements(InlineInfo* inlineInfo, BasicBlock* bloc // Does the local we're about to null out appear in the return // expression? If so we somehow messed up and didn't properly // spill the return value. See impInlineFetchLocal. - GenTree* retExpr = inlineInfo->retExpr; - if (retExpr != nullptr) + if ((inlCandInfo->retExpr != nullptr) && (inlCandInfo->retExpr->gtSubstExpr != nullptr)) { - const bool interferesWithReturn = gtHasRef(inlineInfo->retExpr, tmpNum); + const bool interferesWithReturn = gtHasRef(inlCandInfo->retExpr->gtSubstExpr, tmpNum); noway_assert(!interferesWithReturn); } diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 2321a624fc74a1..2e488de16f8bf8 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -7364,7 +7364,7 @@ GenTreeLclFld* Compiler::gtNewLclFldNode(unsigned lnum, var_types type, unsigned return node; } -GenTree* Compiler::gtNewInlineCandidateReturnExpr(GenTree* inlineCandidate, var_types type, BasicBlockFlags bbFlags) +GenTreeRetExpr* Compiler::gtNewInlineCandidateReturnExpr(GenTreeCall* inlineCandidate, var_types type) { assert(GenTree::s_gtNodeSizes[GT_RET_EXPR] == TREE_NODE_SZ_LARGE); @@ -7372,14 +7372,10 @@ GenTree* Compiler::gtNewInlineCandidateReturnExpr(GenTree* inlineCandidate, var_ node->gtInlineCandidate = inlineCandidate; - node->bbFlags = bbFlags; + node->gtSubstExpr = nullptr; + node->gtSubstBB = nullptr; - if (varTypeIsStruct(inlineCandidate) && !inlineCandidate->OperIsBlkOp()) - { - node->gtRetClsHnd = gtGetStructHandle(inlineCandidate); - } - - // GT_RET_EXPR node eventually might be bashed back to GT_CALL (when inlining is aborted for example). + // GT_RET_EXPR node eventually might be turned back into GT_CALL (when inlining is aborted for example). // Therefore it should carry the GTF_CALL flag so that all the rules about spilling can apply to it as well. // For example, impImportLeave or CEE_POP need to spill GT_RET_EXPR before empty the evaluation stack. node->gtFlags |= GTF_CALL; @@ -17367,7 +17363,7 @@ CORINFO_CLASS_HANDLE Compiler::gtGetStructHandleIfPresent(GenTree* tree) structHnd = tree->AsCall()->gtRetClsHnd; break; case GT_RET_EXPR: - structHnd = tree->AsRetExpr()->gtRetClsHnd; + structHnd = tree->AsRetExpr()->gtInlineCandidate->gtRetClsHnd; break; case GT_FIELD: info.compCompHnd->getFieldType(tree->AsField()->gtFldHnd, &structHnd); diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index 70abd0a98ebb19..04e90363c5248c 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -7234,11 +7234,18 @@ struct GenTreeStoreInd : public GenTreeIndir struct GenTreeRetExpr : public GenTree { - GenTree* gtInlineCandidate; - - BasicBlockFlags bbFlags; - - CORINFO_CLASS_HANDLE gtRetClsHnd; + GenTreeCall* gtInlineCandidate; + + // Expression representing gtInlineCandidate's value (e.g. spill temp or + // expression from inlinee, or call itself for unsuccessful inlines). + // Substituted by UpdateInlineReturnExpressionPlaceHolder. + // This tree is nullptr during the import that created the GenTreeRetExpr and is set later + // when handling the actual inline candidate. + GenTree* gtSubstExpr; + + // The basic block that gtSubstExpr comes from, to enable propagating mandatory flags. + // nullptr for cases where gtSubstExpr is not a tree from the inlinee. + BasicBlock* gtSubstBB; GenTreeRetExpr(var_types type) : GenTree(GT_RET_EXPR, type) { diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index f2ac7b70511bad..1e623845cf2d5c 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -11294,13 +11294,15 @@ bool Compiler::impReturnInstruction(int prefixFlags, OPCODE& opcode) } #endif + InlineCandidateInfo* inlCandInfo = impInlineInfo->inlineCandidateInfo; + GenTreeRetExpr* inlRetExpr = inlCandInfo->retExpr; // Make sure the type matches the original call. var_types returnType = genActualType(op2->gtType); - var_types originalCallType = impInlineInfo->inlineCandidateInfo->fncRetType; + var_types originalCallType = inlCandInfo->fncRetType; if ((returnType != originalCallType) && (originalCallType == TYP_STRUCT)) { - originalCallType = impNormStructType(impInlineInfo->inlineCandidateInfo->methInfo.args.retTypeClass); + originalCallType = impNormStructType(inlCandInfo->methInfo.args.retTypeClass); } if (returnType != originalCallType) @@ -11370,7 +11372,7 @@ bool Compiler::impReturnInstruction(int prefixFlags, OPCODE& opcode) bool isNonNull = false; CORINFO_CLASS_HANDLE returnClsHnd = gtGetClassHandle(op2, &isExact, &isNonNull); - if (impInlineInfo->retExpr == nullptr) + if (inlRetExpr->gtSubstExpr == nullptr) { // This is the first return, so best known type is the type // of this return value. @@ -11394,12 +11396,12 @@ bool Compiler::impReturnInstruction(int prefixFlags, OPCODE& opcode) op2 = tmpOp2; #ifdef DEBUG - if (impInlineInfo->retExpr) + if (inlRetExpr->gtSubstExpr != nullptr) { // Some other block(s) have seen the CEE_RET first. // Better they spilled to the same temp. - assert(impInlineInfo->retExpr->gtOper == GT_LCL_VAR); - assert(impInlineInfo->retExpr->AsLclVarCommon()->GetLclNum() == + assert(inlRetExpr->gtSubstExpr->gtOper == GT_LCL_VAR); + assert(inlRetExpr->gtSubstExpr->AsLclVarCommon()->GetLclNum() == op2->AsLclVarCommon()->GetLclNum()); } #endif @@ -11414,7 +11416,8 @@ bool Compiler::impReturnInstruction(int prefixFlags, OPCODE& opcode) #endif // Report the return expression - impInlineInfo->retExpr = op2; + inlRetExpr->gtSubstExpr = op2; + inlRetExpr->gtSubstBB = fgNeedReturnSpillTemp() ? nullptr : compCurBB; } else { @@ -11440,16 +11443,17 @@ bool Compiler::impReturnInstruction(int prefixFlags, OPCODE& opcode) if (fgNeedReturnSpillTemp()) { - if (impInlineInfo->retExpr == nullptr) + if (inlRetExpr->gtSubstExpr == nullptr) { // The inlinee compiler has figured out the type of the temp already. Use it here. - impInlineInfo->retExpr = + inlRetExpr->gtSubstExpr = gtNewLclvNode(lvaInlineeReturnSpillTemp, lvaTable[lvaInlineeReturnSpillTemp].lvType); } } else { - impInlineInfo->retExpr = op2; + inlRetExpr->gtSubstExpr = op2; + inlRetExpr->gtSubstBB = compCurBB; } } else // The struct was to be returned via a return buffer. @@ -11460,24 +11464,20 @@ bool Compiler::impReturnInstruction(int prefixFlags, OPCODE& opcode) if (fgNeedReturnSpillTemp()) { // If this is the first return we have seen set the retExpr. - if (impInlineInfo->retExpr == nullptr) + if (inlRetExpr->gtSubstExpr == nullptr) { - impInlineInfo->retExpr = + inlRetExpr->gtSubstExpr = impAssignStructPtr(dest, gtNewLclvNode(lvaInlineeReturnSpillTemp, info.compRetType), retClsHnd, CHECK_SPILL_ALL); } } else { - impInlineInfo->retExpr = impAssignStructPtr(dest, op2, retClsHnd, CHECK_SPILL_ALL); + inlRetExpr->gtSubstExpr = impAssignStructPtr(dest, op2, retClsHnd, CHECK_SPILL_ALL); + inlRetExpr->gtSubstBB = compCurBB; } } } - - if (impInlineInfo->retExpr != nullptr) - { - impInlineInfo->retBB = compCurBB; - } } } diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index 78f05c62192fd4..bfd042fd102e83 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -1389,7 +1389,7 @@ var_types Compiler::impImportCall(OPCODE opcode, impAppendTree(call, CHECK_SPILL_ALL, impCurStmtDI, false); // TODO: Still using the widened type. - GenTree* retExpr = gtNewInlineCandidateReturnExpr(call, genActualType(callRetTyp), compCurBB->bbFlags); + GenTreeRetExpr* retExpr = gtNewInlineCandidateReturnExpr(call->AsCall(), genActualType(callRetTyp)); // Link the retExpr to the call so if necessary we can manipulate it later. origCall->gtInlineCandidateInfo->retExpr = retExpr; diff --git a/src/coreclr/jit/indirectcalltransformer.cpp b/src/coreclr/jit/indirectcalltransformer.cpp index 6af4872473855d..71bcb61f58d935 100644 --- a/src/coreclr/jit/indirectcalltransformer.cpp +++ b/src/coreclr/jit/indirectcalltransformer.cpp @@ -707,15 +707,8 @@ class IndirectCallTransformer // Note implicit by-ref returns should have already been converted // so any struct copy we induce here should be cheap. InlineCandidateInfo* const inlineInfo = origCall->gtInlineCandidateInfo; - GenTree* const retExpr = inlineInfo->retExpr; - // Sanity check the ret expr if non-null: it should refer to the original call. - if (retExpr != nullptr) - { - assert(retExpr->AsRetExpr()->gtInlineCandidate == origCall); - } - - if (origCall->TypeGet() != TYP_VOID) + if (!origCall->TypeIs(TYP_VOID)) { // If there's a spill temp already associated with this inline candidate, // use that instead of allocating a new temp. @@ -763,12 +756,12 @@ class IndirectCallTransformer GenTree* tempTree = compiler->gtNewLclvNode(returnTemp, origCall->TypeGet()); - JITDUMP("Bashing GT_RET_EXPR [%06u] to refer to temp V%02u\n", compiler->dspTreeID(retExpr), + JITDUMP("Linking GT_RET_EXPR [%06u] to refer to temp V%02u\n", compiler->dspTreeID(inlineInfo->retExpr), returnTemp); - retExpr->ReplaceWith(tempTree, compiler); + inlineInfo->retExpr->gtSubstExpr = tempTree; } - else if (retExpr != nullptr) + else if (inlineInfo->retExpr != nullptr) { // We still oddly produce GT_RET_EXPRs for some void // returning calls. Just bash the ret expr to a NOP. @@ -777,8 +770,9 @@ class IndirectCallTransformer // benefit they provide is stitching back larger trees for failed inlines // of void-returning methods. But then the calls likely sit in commas and // the benefit of a larger tree is unclear. - JITDUMP("Bashing GT_RET_EXPR [%06u] for VOID return to NOP\n", compiler->dspTreeID(retExpr)); - retExpr->gtBashToNOP(); + JITDUMP("Linking GT_RET_EXPR [%06u] for VOID return to NOP\n", + compiler->dspTreeID(inlineInfo->retExpr)); + inlineInfo->retExpr->gtSubstExpr = compiler->gtNewNothingNode(); } else { @@ -918,7 +912,7 @@ class IndirectCallTransformer // Re-establish this call as an inline candidate. // - GenTree* oldRetExpr = inlineInfo->retExpr; + GenTreeRetExpr* oldRetExpr = inlineInfo->retExpr; inlineInfo->clsHandle = compiler->info.compCompHnd->getMethodClass(methodHnd); inlineInfo->exactContextHnd = context; inlineInfo->preexistingSpillTemp = returnTemp; @@ -927,24 +921,24 @@ class IndirectCallTransformer // If there was a ret expr for this call, we need to create a new one // and append it just after the call. // - // Note the original GT_RET_EXPR has been bashed to a temp. + // Note the original GT_RET_EXPR has been linked to a temp. // we set all this up in FixupRetExpr(). if (oldRetExpr != nullptr) { - GenTree* retExpr = - compiler->gtNewInlineCandidateReturnExpr(call, call->TypeGet(), thenBlock->bbFlags); - inlineInfo->retExpr = retExpr; + inlineInfo->retExpr = compiler->gtNewInlineCandidateReturnExpr(call, call->TypeGet()); + + GenTree* newRetExpr = inlineInfo->retExpr; if (returnTemp != BAD_VAR_NUM) { - retExpr = compiler->gtNewTempAssign(returnTemp, retExpr); + newRetExpr = compiler->gtNewTempAssign(returnTemp, newRetExpr); } else { // We should always have a return temp if we return results by value assert(origCall->TypeGet() == TYP_VOID); } - compiler->fgNewStmtAtEnd(thenBlock, retExpr); + compiler->fgNewStmtAtEnd(thenBlock, newRetExpr); } } } diff --git a/src/coreclr/jit/inline.h b/src/coreclr/jit/inline.h index 753711cd279a19..3b3a35dce39489 100644 --- a/src/coreclr/jit/inline.h +++ b/src/coreclr/jit/inline.h @@ -606,15 +606,22 @@ struct GuardedDevirtualizationCandidateInfo : HandleHistogramProfileCandidateInf // struct InlineCandidateInfo : public GuardedDevirtualizationCandidateInfo { - CORINFO_METHOD_INFO methInfo; - CORINFO_METHOD_HANDLE ilCallerHandle; // the logical IL caller of this inlinee. + CORINFO_METHOD_INFO methInfo; + + // the logical IL caller of this inlinee. + CORINFO_METHOD_HANDLE ilCallerHandle; CORINFO_CLASS_HANDLE clsHandle; CORINFO_CONTEXT_HANDLE exactContextHnd; - GenTree* retExpr; - unsigned preexistingSpillTemp; - unsigned clsAttr; - unsigned methAttr; - IL_OFFSET ilOffset; // actual IL offset of instruction that resulted in this inline candidate + + // The GT_RET_EXPR node linking back to the inline candidate. + GenTreeRetExpr* retExpr; + + unsigned preexistingSpillTemp; + unsigned clsAttr; + unsigned methAttr; + + // actual IL offset of instruction that resulted in this inline candidate + IL_OFFSET ilOffset; CorInfoInitClassResult initClassResult; var_types fncRetType; bool exactContextNeedsRuntimeLookup; @@ -678,8 +685,6 @@ struct InlineInfo InlineResult* inlineResult; - GenTree* retExpr; // The return expression of the inlined candidate. - BasicBlock* retBB; // The basic block of the return expression of the inlined candidate. CORINFO_CLASS_HANDLE retExprClassHnd; bool retExprClassHndIsExact; diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index d4eee4b3d21c30..cec047e99bb04f 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -5526,9 +5526,13 @@ void Compiler::fgMorphCallInline(GenTreeCall* call, InlineResult* inlineResult) { bool inliningFailed = false; + InlineCandidateInfo* inlCandInfo = call->gtInlineCandidateInfo; + // Is this call an inline candidate? if (call->IsInlineCandidate()) { + inlCandInfo = call->gtInlineCandidateInfo; + InlineContext* createdContext = nullptr; // Attempt the inline fgMorphCallInlineHelper(call, inlineResult, &createdContext); @@ -5582,6 +5586,7 @@ void Compiler::fgMorphCallInline(GenTreeCall* call, InlineResult* inlineResult) // Detach the GT_CALL tree from the original statement by // hanging a "nothing" node to it. Later the "nothing" node will be removed // and the original GT_CALL tree will be picked up by the GT_RET_EXPR node. + inlCandInfo->retExpr->gtSubstExpr = call; noway_assert(fgMorphStmt->GetRootNode() == call); fgMorphStmt->SetRootNode(gtNewNothingNode()); @@ -5685,23 +5690,7 @@ void Compiler::fgMorphCallInlineHelper(GenTreeCall* call, InlineResult* result, } lvaCount = startVars; - -#ifdef DEBUG - if (verbose) - { - // printf("Inlining failed. Restore lvaCount to %d.\n", lvaCount); - } -#endif - - return; - } - -#ifdef DEBUG - if (verbose) - { - // printf("After inlining lvaCount=%d.\n", lvaCount); } -#endif } //------------------------------------------------------------------------ From 177fe72dca673f04b63e378f8a78c419e3dfff80 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 18 Oct 2022 19:18:20 +0200 Subject: [PATCH 2/6] Nit --- src/coreclr/jit/morph.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index cec047e99bb04f..d82a9da209d9dd 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -5531,8 +5531,6 @@ void Compiler::fgMorphCallInline(GenTreeCall* call, InlineResult* inlineResult) // Is this call an inline candidate? if (call->IsInlineCandidate()) { - inlCandInfo = call->gtInlineCandidateInfo; - InlineContext* createdContext = nullptr; // Attempt the inline fgMorphCallInlineHelper(call, inlineResult, &createdContext); From 82b6655bf34c4fc48f47b8c6a49b30776c9110a1 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 18 Oct 2022 19:46:16 +0200 Subject: [PATCH 3/6] Fix build --- src/coreclr/jit/fginline.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/fginline.cpp b/src/coreclr/jit/fginline.cpp index 0f9e8c3d184cfd..d69a4141c237f3 100644 --- a/src/coreclr/jit/fginline.cpp +++ b/src/coreclr/jit/fginline.cpp @@ -281,13 +281,13 @@ class SubstitutePlaceholdersAndDevirtualizeWalker : public GenTreeVisitorOperIs(GT_ASG)) { // The inlinee can only be the RHS. - assert(parent->gtGetOp2() == tree); + assert(parent->gtGetOp2() == *use); AttachStructInlineeToAsg(parent->AsOp(), retClsHnd); } else { // Just assign the inlinee to a variable to keep it simple. - *use = AssignStructInlineeToVar(tree, retClsHnd); + *use = AssignStructInlineeToVar(*use, retClsHnd); } m_madeChanges = true; } From 37086f7145882d57759c82ed46ea034a27e3f6a0 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Wed, 19 Oct 2022 11:48:44 +0200 Subject: [PATCH 4/6] Propagate BB flags properly on inline failures When we fail to inline, the substitution will be the call itself, so we should propagate flags from the basic block containing the call. In particular this can be a problem with RET_EXPR chains. --- src/coreclr/jit/morph.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index d82a9da209d9dd..b00f2a21326653 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -5585,6 +5585,7 @@ void Compiler::fgMorphCallInline(GenTreeCall* call, InlineResult* inlineResult) // hanging a "nothing" node to it. Later the "nothing" node will be removed // and the original GT_CALL tree will be picked up by the GT_RET_EXPR node. inlCandInfo->retExpr->gtSubstExpr = call; + inlCandInfo->retExpr->gtSubstBB = compCurBB; noway_assert(fgMorphStmt->GetRootNode() == call); fgMorphStmt->SetRootNode(gtNewNothingNode()); From 802ac66494f9d41a7b869b598ed333c79b2679c3 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Wed, 19 Oct 2022 14:31:09 +0200 Subject: [PATCH 5/6] Fix dumping, retain odd previous behavior, fix GDV substitution --- src/coreclr/jit/fginline.cpp | 27 ++++++++++++++++--- src/coreclr/jit/gentree.cpp | 12 ++++++--- src/coreclr/jit/importer.cpp | 21 ++++++++++++--- src/coreclr/jit/indirectcalltransformer.cpp | 29 ++++++++++++++------- src/coreclr/jit/morph.cpp | 2 +- 5 files changed, 71 insertions(+), 20 deletions(-) diff --git a/src/coreclr/jit/fginline.cpp b/src/coreclr/jit/fginline.cpp index d69a4141c237f3..0f0953a195490f 100644 --- a/src/coreclr/jit/fginline.cpp +++ b/src/coreclr/jit/fginline.cpp @@ -194,12 +194,14 @@ class SubstitutePlaceholdersAndDevirtualizeWalker : public GenTreeVisitorAsRetExpr(); inlineCandidate = retExpr->gtSubstExpr; + prevInlineeBB = inlineeBB; inlineeBB = retExpr->gtSubstBB; } while (inlineCandidate->OperIs(GT_RET_EXPR)); @@ -240,16 +242,33 @@ class SubstitutePlaceholdersAndDevirtualizeWalker : public GenTreeVisitorcompCurBB->bbFlags |= (inlineeBB->bbFlags & BBF_SPLIT_GAINED); + newBBFlags |= prevInlineeBB->bbFlags; + + if (inlineeBB != nullptr) + { + newBBFlags |= inlineeBB->bbFlags & BBF_COPY_PROPAGATE; + } } + else if (inlineeBB != nullptr) + { + newBBFlags |= inlineeBB->bbFlags; + } + + m_compiler->compCurBB->bbFlags |= (newBBFlags & BBF_SPLIT_GAINED); #ifdef DEBUG if (m_compiler->verbose) { printf("\nInserting the inline return expression\n"); - m_compiler->gtDispTree(tree); + m_compiler->gtDispTree(inlineCandidate); printf("\n"); } #endif // DEBUG diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 2e488de16f8bf8..e3d52047170f65 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -11480,10 +11480,16 @@ void Compiler::gtDispLeaf(GenTree* tree, IndentStack* indentStack) case GT_RET_EXPR: { - GenTree* const associatedTree = tree->AsRetExpr()->gtInlineCandidate; - printf("(inl return %s ", tree->IsCall() ? " from call" : "expr"); - printTreeID(associatedTree); + GenTreeCall* inlineCand = tree->AsRetExpr()->gtInlineCandidate; + printf("(for "); + printTreeID(inlineCand); printf(")"); + + if (tree->AsRetExpr()->gtSubstExpr != nullptr) + { + printf(" -> "); + printTreeID(tree->AsRetExpr()->gtSubstExpr); + } } break; diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 1e623845cf2d5c..9495b45b817613 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -11417,7 +11417,6 @@ bool Compiler::impReturnInstruction(int prefixFlags, OPCODE& opcode) // Report the return expression inlRetExpr->gtSubstExpr = op2; - inlRetExpr->gtSubstBB = fgNeedReturnSpillTemp() ? nullptr : compCurBB; } else { @@ -11453,7 +11452,6 @@ bool Compiler::impReturnInstruction(int prefixFlags, OPCODE& opcode) else { inlRetExpr->gtSubstExpr = op2; - inlRetExpr->gtSubstBB = compCurBB; } } else // The struct was to be returned via a return buffer. @@ -11474,10 +11472,27 @@ bool Compiler::impReturnInstruction(int prefixFlags, OPCODE& opcode) else { inlRetExpr->gtSubstExpr = impAssignStructPtr(dest, op2, retClsHnd, CHECK_SPILL_ALL); - inlRetExpr->gtSubstBB = compCurBB; } } } + + // TODO-Inlining: Setting gtSubstBB unconditionally here does not + // make sense. The intention is to be able to propagate BB flags in + // UpdateInlineReturnExpressionPlaceHolder, but we only need to + // propagate the mandatory flags in case gtSubstExpr is a tree + // (e.g. GT_ARR_LENGTH in gtSubstExpr means we need to set + // BBF_HAS_IDX_LEN on the final BB that the substituted expression + // ends up in -- so we should not need to set this for the spill + // temp cases above). + // + // However, during substitution we actually propagate all + // BBF_SPLIT_GAINED flags which includes BBF_PROF_WEIGHT. The net + // effect of this is that if we inline a tree from a hot block into + // a block without profile data we suddenly start to believe the + // inliner block is hot, and then future inline candidates are + // treated more aggressively. Changing this leads to large diffs. + assert(inlRetExpr->gtSubstExpr != nullptr); + inlRetExpr->gtSubstBB = compCurBB; } } diff --git a/src/coreclr/jit/indirectcalltransformer.cpp b/src/coreclr/jit/indirectcalltransformer.cpp index 71bcb61f58d935..894cdd5ab58593 100644 --- a/src/coreclr/jit/indirectcalltransformer.cpp +++ b/src/coreclr/jit/indirectcalltransformer.cpp @@ -1106,10 +1106,10 @@ class IndirectCallTransformer unsigned chainLikelihood = 0; GenTreeCall* chainedCall = nullptr; - // Helper class to check a statement for uncloneable nodes and count + // Helper class to check/fix a statement for clonability and count // the total number of nodes // - class UnclonableVisitor final : public GenTreeVisitor + class ClonabilityVisitor final : public GenTreeVisitor { public: enum @@ -1120,8 +1120,8 @@ class IndirectCallTransformer GenTree* m_unclonableNode; unsigned m_nodeCount; - UnclonableVisitor(Compiler* compiler) - : GenTreeVisitor(compiler), m_unclonableNode(nullptr), m_nodeCount(0) + ClonabilityVisitor(Compiler* compiler) + : GenTreeVisitor(compiler), m_unclonableNode(nullptr), m_nodeCount(0) { } @@ -1141,6 +1141,16 @@ class IndirectCallTransformer } else if (node->OperIs(GT_RET_EXPR)) { + // If this is a RET_EXPR that we already know how to substitute then it is the + // "fixed-up" RET_EXPR from a previous GDV candidate. In that case we can + // substitute it right here to make it eligibile for cloning. + if (node->AsRetExpr()->gtSubstExpr != nullptr) + { + assert(node->AsRetExpr()->gtInlineCandidate->IsGuarded()); + *use = node->AsRetExpr()->gtSubstExpr; + return fgWalkResult::WALK_CONTINUE; + } + m_unclonableNode = node; return fgWalkResult::WALK_ABORT; } @@ -1186,19 +1196,20 @@ class IndirectCallTransformer // See if this statement's tree is one that we can clone. // - UnclonableVisitor unclonableVisitor(compiler); - unclonableVisitor.WalkTree(nextStmt->GetRootNodePointer(), nullptr); + ClonabilityVisitor clonabilityVisitor(compiler); + clonabilityVisitor.WalkTree(nextStmt->GetRootNodePointer(), nullptr); - if (unclonableVisitor.m_unclonableNode != nullptr) + if (clonabilityVisitor.m_unclonableNode != nullptr) { - JITDUMP(" node [%06u] can't be cloned\n", compiler->dspTreeID(unclonableVisitor.m_unclonableNode)); + JITDUMP(" node [%06u] can't be cloned\n", + compiler->dspTreeID(clonabilityVisitor.m_unclonableNode)); break; } // Looks like we can clone this, so keep scouting. // chainStatementDup++; - chainNodeDup += unclonableVisitor.m_nodeCount; + chainNodeDup += clonabilityVisitor.m_nodeCount; } } diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index b00f2a21326653..b7b4766b387d5f 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -5585,7 +5585,7 @@ void Compiler::fgMorphCallInline(GenTreeCall* call, InlineResult* inlineResult) // hanging a "nothing" node to it. Later the "nothing" node will be removed // and the original GT_CALL tree will be picked up by the GT_RET_EXPR node. inlCandInfo->retExpr->gtSubstExpr = call; - inlCandInfo->retExpr->gtSubstBB = compCurBB; + inlCandInfo->retExpr->gtSubstBB = compCurBB; noway_assert(fgMorphStmt->GetRootNode() == call); fgMorphStmt->SetRootNode(gtNewNothingNode()); From 0177368f44a61543e2bacf1cbb7c73217bb746f5 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Wed, 19 Oct 2022 14:34:20 +0200 Subject: [PATCH 6/6] More comments --- src/coreclr/jit/fginline.cpp | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/coreclr/jit/fginline.cpp b/src/coreclr/jit/fginline.cpp index 0f0953a195490f..55443f8d0a2b70 100644 --- a/src/coreclr/jit/fginline.cpp +++ b/src/coreclr/jit/fginline.cpp @@ -247,6 +247,13 @@ class SubstitutePlaceholdersAndDevirtualizeWalker : public GenTreeVisitor