Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

JIT: Clean up inliner substitution #77176

Merged
merged 6 commits into from
Oct 21, 2022
Merged

JIT: Clean up inliner substitution #77176

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5f164ff
JIT: Clean up inliner substitution
jakobbotsch Oct 18, 2022
177fe72
Nit
jakobbotsch Oct 18, 2022
82b6655
Fix build
jakobbotsch Oct 18, 2022
37086f7
Propagate BB flags properly on inline failures
jakobbotsch Oct 19, 2022
802ac66
Fix dumping, retain odd previous behavior, fix GDV substitution
jakobbotsch Oct 19, 2022
0177368
More comments
jakobbotsch Oct 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/coreclr/jit/compiler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
114 changes: 55 additions & 59 deletions src/coreclr/jit/fginline.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ class SubstitutePlaceholdersAndDevirtualizeWalker : public GenTreeVisitor<Substi

if (tree->OperIs(GT_RET_EXPR))
{
UpdateInlineReturnExpressionPlaceHolder(tree, user);
UpdateInlineReturnExpressionPlaceHolder(use, user);
}

#if FEATURE_MULTIREG_RET
Expand All @@ -121,9 +121,10 @@ class SubstitutePlaceholdersAndDevirtualizeWalker : public GenTreeVisitor<Substi
{
GenTree* effectiveValue = value->gtEffectiveVal(/*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));
}
}

Expand All @@ -144,8 +145,8 @@ class SubstitutePlaceholdersAndDevirtualizeWalker : public GenTreeVisitor<Substi
// inline return expression placeholder if there is one.
//
// Arguments:
// pTree -- pointer to tree to examine for updates
// data -- context data for the tree walk
// use -- edge for the tree that is a GT_RET_EXPR node
// parent -- node containing the edge
//
// Returns:
// fgWalkResult indicating the walk should continue; that
Expand Down Expand Up @@ -175,30 +176,33 @@ class SubstitutePlaceholdersAndDevirtualizeWalker : public GenTreeVisitor<Substi
// the functions makes necessary updates. It could happen if there was
// an implicit conversion in the inlinee body.
//
void UpdateInlineReturnExpressionPlaceHolder(GenTree* tree, GenTree* parent)
void UpdateInlineReturnExpressionPlaceHolder(GenTree** use, GenTree* parent)
{
CORINFO_CLASS_HANDLE retClsHnd = NO_CLASS_HANDLE;

while (tree->OperIs(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* prevInlineeBB = nullptr;
BasicBlock* inlineeBB = nullptr;
GenTree* inlineCandidate = tree;
do
{
GenTreeRetExpr* retExpr = inlineCandidate->AsRetExpr();
inlineCandidate = retExpr->gtInlineCandidate;
bbFlags = retExpr->bbFlags;
inlineCandidate = retExpr->gtSubstExpr;
prevInlineeBB = inlineeBB;
inlineeBB = retExpr->gtSubstBB;
} while (inlineCandidate->OperIs(GT_RET_EXPR));

// We might as well try and fold the return value. Eg returns of
Expand Down Expand Up @@ -235,15 +239,43 @@ class SubstitutePlaceholdersAndDevirtualizeWalker : public GenTreeVisitor<Substi
}
}

tree->ReplaceWith(inlineCandidate, m_compiler);
*use = inlineCandidate;
m_madeChanges = true;
m_compiler->compCurBB->bbFlags |= (bbFlags & BBF_SPLIT_GAINED);

// TODO-Inlining: The inliner had strange previous behavior where
// it for successful inlines that were chains would propagate the
// flags from the second-to-last inlinee's BB. Remove this once we
// can take these changes. We still need to propagate the mandatory
// "IR-presence" flags.
// Furthermore, we should really only propagate BBF_COPY_PROPAGATE
// flags here. BBF_SPLIT_GAINED includes BBF_PROF_WEIGHT, and
// propagating that has the effect that inlining a tree from a hot
// block into a block without profile weights means we suddenly
// start to see the inliner block as hot and treat future inline
// candidates more aggressively.
//
BasicBlockFlags newBBFlags = BBF_EMPTY;
if (prevInlineeBB != nullptr)
{
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
Expand Down Expand Up @@ -275,13 +307,13 @@ class SubstitutePlaceholdersAndDevirtualizeWalker : public GenTreeVisitor<Substi
if (parent->OperIs(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.
tree->ReplaceWith(AssignStructInlineeToVar(tree, retClsHnd), m_compiler);
*use = AssignStructInlineeToVar(*use, retClsHnd);
}
m_madeChanges = true;
}
Expand Down Expand Up @@ -853,8 +885,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;
Expand Down Expand Up @@ -1006,12 +1036,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)
Expand Down Expand Up @@ -1386,40 +1416,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.
Expand Down Expand Up @@ -1839,6 +1835,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++)
{
Expand Down Expand Up @@ -1873,10 +1870,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);
}

Expand Down
26 changes: 14 additions & 12 deletions src/coreclr/jit/gentree.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7364,22 +7364,18 @@ 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);

GenTreeRetExpr* node = new (this, GT_RET_EXPR) GenTreeRetExpr(type);

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;
Expand Down Expand Up @@ -11484,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;

Expand Down Expand Up @@ -17367,7 +17369,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);
Expand Down
17 changes: 12 additions & 5 deletions src/coreclr/jit/gentree.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down
Loading