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

Fix propagation of basic block flags for inlinee return expressions. #37840

Merged
merged 1 commit into from
Jun 15, 2020
Merged

Fix propagation of basic block flags for inlinee return expressions. #37840

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 5 additions & 4 deletions src/coreclr/src/jit/compiler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4198,10 +4198,11 @@ class Compiler
InlineCandidateInfo** ppInlineCandidateInfo,
InlineResult* inlineResult);

void impInlineRecordArgInfo(InlineInfo* pInlineInfo,
GenTree* curArgVal,
unsigned argNum,
InlineResult* inlineResult);
void impInlineRecordArgInfo(InlineInfo* pInlineInfo,
GenTree* curArgVal,
unsigned argNum,
unsigned __int64 bbFlags,
InlineResult* inlineResult);

void impInlineInitVars(InlineInfo* pInlineInfo);

Expand Down
6 changes: 5 additions & 1 deletion src/coreclr/src/jit/flowgraph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23643,6 +23643,7 @@ Statement* Compiler::fgInlinePrependStatements(InlineInfo* inlineInfo)
const InlArgInfo& argInfo = inlArgInfo[argNum];
const bool argIsSingleDef = !argInfo.argHasLdargaOp && !argInfo.argHasStargOp;
GenTree* const argNode = inlArgInfo[argNum].argNode;
unsigned __int64 bbFlags = inlArgInfo[argNum].bbFlags;

if (argInfo.argHasTmp)
{
Expand Down Expand Up @@ -23705,6 +23706,7 @@ Statement* Compiler::fgInlinePrependStatements(InlineInfo* inlineInfo)
}
#endif // DEBUG
}
block->bbFlags |= (bbFlags & BBF_SPLIT_GAINED);
}
else if (argInfo.argIsByRefToStructLocal)
{
Expand Down Expand Up @@ -23758,7 +23760,7 @@ Statement* Compiler::fgInlinePrependStatements(InlineInfo* inlineInfo)
//
// Chase through any GT_RET_EXPRs to find the actual argument
// expression.
GenTree* actualArgNode = argNode->gtRetExprVal();
GenTree* actualArgNode = argNode->gtRetExprVal(&bbFlags);

// For case (1)
//
Expand Down Expand Up @@ -23834,6 +23836,8 @@ Statement* Compiler::fgInlinePrependStatements(InlineInfo* inlineInfo)
// since the box itself will be ignored.
gtTryRemoveBoxUpstreamEffects(argNode);
}

block->bbFlags |= (bbFlags & BBF_SPLIT_GAINED);
}
}
}
Expand Down
25 changes: 9 additions & 16 deletions src/coreclr/src/jit/gentree.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1707,7 +1707,7 @@ struct GenTree
inline GenTree* gtEffectiveVal(bool commaOnly = false);

// Tunnel through any GT_RET_EXPRs
inline GenTree* gtRetExprVal(unsigned __int64* pbbFlags = nullptr);
inline GenTree* gtRetExprVal(unsigned __int64* pbbFlags);

// Return the child of this node if it is a GT_RELOAD or GT_COPY; otherwise simply return the node itself
inline GenTree* gtSkipReloadOrCopy();
Expand Down Expand Up @@ -7053,23 +7053,16 @@ inline GenTree* GenTree::gtRetExprVal(unsigned __int64* pbbFlags)
GenTree* retExprVal = this;
unsigned __int64 bbFlags = 0;

for (;;)
assert(pbbFlags != nullptr);

for (; retExprVal->gtOper == GT_RET_EXPR; retExprVal = retExprVal->AsRetExpr()->gtInlineCandidate)
{
if (retExprVal->gtOper == GT_RET_EXPR)
{
GenTreeRetExpr* retExp = retExprVal->AsRetExpr();
retExprVal = retExp->gtInlineCandidate;
bbFlags = retExp->bbFlags;
}
else
{
if (pbbFlags != nullptr)
{
*pbbFlags = bbFlags;
}
return retExprVal;
}
bbFlags = retExprVal->AsRetExpr()->bbFlags;
}

*pbbFlags = bbFlags;

return retExprVal;
}

inline GenTree* GenTree::gtSkipReloadOrCopy()
Expand Down
22 changes: 12 additions & 10 deletions src/coreclr/src/jit/importer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18856,10 +18856,8 @@ void Compiler::impCheckCanInline(GenTreeCall* call,
// properties are used later by impInlineFetchArg to determine how best to
// pass the argument into the inlinee.

void Compiler::impInlineRecordArgInfo(InlineInfo* pInlineInfo,
GenTree* curArgVal,
unsigned argNum,
InlineResult* inlineResult)
void Compiler::impInlineRecordArgInfo(
InlineInfo* pInlineInfo, GenTree* curArgVal, unsigned argNum, unsigned __int64 bbFlags, InlineResult* inlineResult)
{
InlArgInfo* inlCurArgInfo = &pInlineInfo->inlArgInfo[argNum];

Expand All @@ -18870,6 +18868,7 @@ void Compiler::impInlineRecordArgInfo(InlineInfo* pInlineInfo,
}

inlCurArgInfo->argNode = curArgVal;
inlCurArgInfo->bbFlags = bbFlags;

GenTree* lclVarTree;

Expand Down Expand Up @@ -19026,9 +19025,10 @@ void Compiler::impInlineInitVars(InlineInfo* pInlineInfo)

if (thisArg)
{
inlArgInfo[0].argIsThis = true;
GenTree* actualThisArg = thisArg->GetNode()->gtRetExprVal();
impInlineRecordArgInfo(pInlineInfo, actualThisArg, argCnt, inlineResult);
inlArgInfo[0].argIsThis = true;
unsigned __int64 bbFlags = 0;
GenTree* actualThisArg = thisArg->GetNode()->gtRetExprVal(&bbFlags);
impInlineRecordArgInfo(pInlineInfo, actualThisArg, argCnt, bbFlags, inlineResult);

if (inlineResult->IsFailure())
{
Expand Down Expand Up @@ -19063,8 +19063,9 @@ void Compiler::impInlineInitVars(InlineInfo* pInlineInfo)
continue;
}

GenTree* actualArg = use.GetNode()->gtRetExprVal();
impInlineRecordArgInfo(pInlineInfo, actualArg, argCnt, inlineResult);
unsigned __int64 bbFlags = 0;
GenTree* actualArg = use.GetNode()->gtRetExprVal(&bbFlags);
impInlineRecordArgInfo(pInlineInfo, actualArg, argCnt, bbFlags, inlineResult);

if (inlineResult->IsFailure())
{
Expand Down Expand Up @@ -20295,7 +20296,8 @@ void Compiler::impDevirtualizeCall(GenTreeCall* call,
if ((objClass == nullptr) || !isExact)
{
// Walk back through any return expression placeholders
actualThisObj = thisObj->gtRetExprVal();
unsigned __int64 bbFlags = 0;
actualThisObj = thisObj->gtRetExprVal(&bbFlags);

// See if we landed on a call to a special intrinsic method
if (actualThisObj->IsCall())
Expand Down
2 changes: 2 additions & 0 deletions src/coreclr/src/jit/inline.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -541,6 +541,8 @@ struct InlineCandidateInfo : public GuardedDevirtualizationCandidateInfo

struct InlArgInfo
{
unsigned __int64 bbFlags; // basic block flags that need to be added when replacing GT_RET_EXPR
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I'd place this field first to minimize the amount of padding we might end up with here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, done.

// with argNode
GenTree* argNode; // caller node for this argument
GenTree* argBashTmpNode; // tmp node created, if it may be replaced with actual arg
unsigned argTmpNum; // the argument tmp number
Expand Down