From c1f9e256c76bfaa1e36773cc5194fc59520615b7 Mon Sep 17 00:00:00 2001 From: Eugene Rozenfeld Date: Thu, 4 Jun 2020 14:01:32 -0700 Subject: [PATCH] Changes for basic block flags. Fix the code that propagates flags from the basic block of the return expression to the caller's basic block during inlining. We are now properly tracking the basic block of the return expression. Fixes #36588. Don't mark BBJ_RETURN blocks with BBF_BACKWARD_JUMP since they are executed at most once. The first change had a few diffs because we propagated BBF_BACKWARD_JUMP flag. After analyzing them I realized that we shouldn't mark BBJ_RETURN blocks with BBF_BACKWARD_JUMP in the first place. That resulted in some diffs because we are less aggressive with inlining of calls outside of loops. --- src/coreclr/src/jit/flowgraph.cpp | 25 ++++++++++++++++--------- src/coreclr/src/jit/gentree.cpp | 6 ++++-- src/coreclr/src/jit/gentree.h | 23 +++++++++++++++++++---- src/coreclr/src/jit/importer.cpp | 5 +++++ src/coreclr/src/jit/inline.h | 1 + 5 files changed, 45 insertions(+), 15 deletions(-) diff --git a/src/coreclr/src/jit/flowgraph.cpp b/src/coreclr/src/jit/flowgraph.cpp index a30085697644a..3472ce02d3e9c 100644 --- a/src/coreclr/src/jit/flowgraph.cpp +++ b/src/coreclr/src/jit/flowgraph.cpp @@ -5275,7 +5275,7 @@ void Compiler::fgMarkBackwardJump(BasicBlock* targetBlock, BasicBlock* sourceBlo for (BasicBlock* block = targetBlock; block != sourceBlock->bbNext; block = block->bbNext) { - if ((block->bbFlags & BBF_BACKWARD_JUMP) == 0) + if (((block->bbFlags & BBF_BACKWARD_JUMP) == 0) && (block->bbJumpKind != BBJ_RETURN)) { block->bbFlags |= BBF_BACKWARD_JUMP; compHasBackwardJump = true; @@ -22625,9 +22625,10 @@ Compiler::fgWalkResult Compiler::fgUpdateInlineReturnExpressionPlaceHolder(GenTr // This folding may uncover more GT_RET_EXPRs, so we loop around // until we've got something distinct. // - GenTree* inlineCandidate = tree->gtRetExprVal(); - inlineCandidate = comp->gtFoldExpr(inlineCandidate); - var_types retType = tree->TypeGet(); + unsigned __int64 bbFlags = 0; + GenTree* inlineCandidate = tree->gtRetExprVal(&bbFlags); + inlineCandidate = comp->gtFoldExpr(inlineCandidate); + var_types retType = tree->TypeGet(); #ifdef DEBUG if (comp->verbose) @@ -22643,6 +22644,7 @@ Compiler::fgWalkResult Compiler::fgUpdateInlineReturnExpressionPlaceHolder(GenTr #endif // DEBUG tree->ReplaceWith(inlineCandidate, comp); + comp->compCurBB->bbFlags |= (bbFlags & BBF_SPLIT_GAINED); #ifdef DEBUG if (comp->verbose) @@ -22992,6 +22994,7 @@ void Compiler::fgInvokeInlineeCompiler(GenTreeCall* call, InlineResult* inlineRe inlineInfo.iciBlock = compCurBB; inlineInfo.thisDereferencedFirst = false; inlineInfo.retExpr = nullptr; + inlineInfo.retBB = nullptr; inlineInfo.retExprClassHnd = nullptr; inlineInfo.retExprClassHndIsExact = false; inlineInfo.inlineResult = inlineResult; @@ -23355,7 +23358,7 @@ void Compiler::fgInsertInlineeBlocks(InlineInfo* pInlineInfo) // topBlock contains at least one statement before the split. // And the split is before the first statement. - // In this case, topBlock should be empty, and everything else should be moved to the bottonBlock. + // In this case, topBlock should be empty, and everything else should be moved to the bottomBlock. bottomBlock->bbStmtList = topBlock->bbStmtList; topBlock->bbStmtList = nullptr; } @@ -23535,13 +23538,17 @@ void Compiler::fgInsertInlineeBlocks(InlineInfo* pInlineInfo) gtDispTree(pInlineInfo->retExpr); } #endif // DEBUG - // Replace the call with the return expression - iciCall->ReplaceWith(pInlineInfo->retExpr, this); - if (bottomBlock != nullptr) + // 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 fgUpdateInlineReturnExpressionPlaceHolder. 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) { - bottomBlock->bbFlags |= InlineeCompiler->fgLastBB->bbFlags & BBF_SPLIT_GAINED; + // Save the basic block flags from the retExpr basic block. + iciCall->gtInlineCandidateInfo->retExpr->AsRetExpr()->bbFlags = pInlineInfo->retBB->bbFlags; } + iciCall->ReplaceWith(pInlineInfo->retExpr, this); } // diff --git a/src/coreclr/src/jit/gentree.cpp b/src/coreclr/src/jit/gentree.cpp index 3ff1403ed021e..b01906292a148 100644 --- a/src/coreclr/src/jit/gentree.cpp +++ b/src/coreclr/src/jit/gentree.cpp @@ -6263,9 +6263,11 @@ GenTree* Compiler::gtNewInlineCandidateReturnExpr(GenTree* inlineCandidate, var_ { assert(GenTree::s_gtNodeSizes[GT_RET_EXPR] == TREE_NODE_SZ_LARGE); - GenTree* node = new (this, GT_RET_EXPR) GenTreeRetExpr(type); + GenTreeRetExpr* node = new (this, GT_RET_EXPR) GenTreeRetExpr(type); - node->AsRetExpr()->gtInlineCandidate = inlineCandidate; + node->gtInlineCandidate = inlineCandidate; + + node->bbFlags = 0; if (varTypeIsStruct(inlineCandidate) && !inlineCandidate->OperIsBlkOp()) { diff --git a/src/coreclr/src/jit/gentree.h b/src/coreclr/src/jit/gentree.h index ec0cbee87192e..fd196228074de 100644 --- a/src/coreclr/src/jit/gentree.h +++ b/src/coreclr/src/jit/gentree.h @@ -1708,7 +1708,7 @@ struct GenTree inline GenTree* gtEffectiveVal(bool commaOnly = false); // Tunnel through any GT_RET_EXPRs - inline GenTree* gtRetExprVal(); + inline GenTree* gtRetExprVal(unsigned __int64* pbbFlags = nullptr); // Return the child of this node if it is a GT_RELOAD or GT_COPY; otherwise simply return the node itself inline GenTree* gtSkipReloadOrCopy(); @@ -5628,6 +5628,8 @@ struct GenTreeRetExpr : public GenTree { GenTree* gtInlineCandidate; + unsigned __int64 bbFlags; + CORINFO_CLASS_HANDLE gtRetClsHnd; GenTreeRetExpr(var_types type) : GenTree(GT_RET_EXPR, type) @@ -7040,6 +7042,11 @@ inline GenTree* GenTree::gtEffectiveVal(bool commaOnly) //------------------------------------------------------------------------- // gtRetExprVal - walk back through GT_RET_EXPRs // +// Arguments: +// pbbFlags - out-parameter that is set to the flags of the basic block +// containing the inlinee return value. The value is 0 +// for unsuccessful inlines. +// // Returns: // tree representing return value from a successful inline, // or original call for failed or yet to be determined inline. @@ -7048,17 +7055,25 @@ inline GenTree* GenTree::gtEffectiveVal(bool commaOnly) // Multi-level inlines can form chains of GT_RET_EXPRs. // This method walks back to the root of the chain. -inline GenTree* GenTree::gtRetExprVal() +inline GenTree* GenTree::gtRetExprVal(unsigned __int64* pbbFlags) { - GenTree* retExprVal = this; + GenTree* retExprVal = this; + unsigned __int64 bbFlags = 0; + for (;;) { if (retExprVal->gtOper == GT_RET_EXPR) { - retExprVal = retExprVal->AsRetExpr()->gtInlineCandidate; + GenTreeRetExpr* retExp = retExprVal->AsRetExpr(); + retExprVal = retExp->gtInlineCandidate; + bbFlags = retExp->bbFlags; } else { + if (pbbFlags != nullptr) + { + *pbbFlags = bbFlags; + } return retExprVal; } } diff --git a/src/coreclr/src/jit/importer.cpp b/src/coreclr/src/jit/importer.cpp index d5bf9bf745541..079a111b25b98 100644 --- a/src/coreclr/src/jit/importer.cpp +++ b/src/coreclr/src/jit/importer.cpp @@ -16836,6 +16836,11 @@ bool Compiler::impReturnInstruction(int prefixFlags, OPCODE& opcode) } } } + + if (impInlineInfo->retExpr != nullptr) + { + impInlineInfo->retBB = compCurBB; + } } } diff --git a/src/coreclr/src/jit/inline.h b/src/coreclr/src/jit/inline.h index 76011c10028b4..ff046826d4359 100644 --- a/src/coreclr/src/jit/inline.h +++ b/src/coreclr/src/jit/inline.h @@ -584,6 +584,7 @@ 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;