Skip to content

JIT: fix overly aggressive tail recursive call loop marking #33517

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

Merged
merged 3 commits into from
Mar 12, 2020
Merged

JIT: fix overly aggressive tail recursive call loop marking #33517

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
81c9a44
JIT: fix overly aggressive tail recursive call loop marking
AndyAyersMS Mar 12, 2020
9a867cc
fix formatting
AndyAyersMS Mar 12, 2020
1d5c15c
review feedback
AndyAyersMS Mar 12, 2020
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
91 changes: 35 additions & 56 deletions src/coreclr/src/jit/importer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7333,8 +7333,8 @@ var_types Compiler::impImportCall(OPCODE opcode,
bool exactContextNeedsRuntimeLookup = false;
bool canTailCall = true;
const char* szCanTailCallFailReason = nullptr;
int tailCall = prefixFlags & PREFIX_TAILCALL;
bool readonlyCall = (prefixFlags & PREFIX_READONLY) != 0;
const int tailCallFlags = (prefixFlags & PREFIX_TAILCALL);
const bool isReadonlyCall = (prefixFlags & PREFIX_READONLY) != 0;

CORINFO_RESOLVED_TOKEN* ldftnToken = nullptr;

Expand Down Expand Up @@ -7523,10 +7523,11 @@ var_types Compiler::impImportCall(OPCODE opcode,
bool isSpecialIntrinsic = false;
if ((mflags & (CORINFO_FLG_INTRINSIC | CORINFO_FLG_JIT_INTRINSIC)) != 0)
{
const bool isTail = canTailCall && (tailCall != 0);
const bool isTailCall = canTailCall && (tailCallFlags != 0);

call = impIntrinsic(newobjThis, clsHnd, methHnd, sig, mflags, pResolvedToken->token, readonlyCall, isTail,
pConstrainedResolvedToken, callInfo->thisTransform, &intrinsicID, &isSpecialIntrinsic);
call = impIntrinsic(newobjThis, clsHnd, methHnd, sig, mflags, pResolvedToken->token, isReadonlyCall,
isTailCall, pConstrainedResolvedToken, callInfo->thisTransform, &intrinsicID,
&isSpecialIntrinsic);

if (compDonotInline())
{
Expand Down Expand Up @@ -8147,7 +8148,7 @@ var_types Compiler::impImportCall(OPCODE opcode,
return TYP_UNDEF;
}

if ((clsFlags & CORINFO_FLG_ARRAY) && readonlyCall)
if ((clsFlags & CORINFO_FLG_ARRAY) && isReadonlyCall)
{
// We indicate "readonly" to the Address operation by using a null
// instParam.
Expand Down Expand Up @@ -8270,10 +8271,10 @@ var_types Compiler::impImportCall(OPCODE opcode,

// See if we can devirtualize.

bool explicitTailCall = (tailCall & PREFIX_TAILCALL_EXPLICIT) != 0;
const bool isExplicitTailCall = (tailCallFlags & PREFIX_TAILCALL_EXPLICIT) != 0;
const bool isLateDevirtualization = false;
impDevirtualizeCall(call->AsCall(), &callInfo->hMethod, &callInfo->methodFlags, &callInfo->contextHandle,
&exactContextHnd, isLateDevirtualization, explicitTailCall);
&exactContextHnd, isLateDevirtualization, isExplicitTailCall);
}

if (impIsThis(obj))
Expand Down Expand Up @@ -8362,8 +8363,16 @@ var_types Compiler::impImportCall(OPCODE opcode,

DONE:

if (tailCall)
// Final importer checks for calls flagged as tail calls.
//
if (tailCallFlags != 0)
{
const bool isExplicitTailCall = (tailCallFlags & PREFIX_TAILCALL_EXPLICIT) != 0;
const bool isImplicitTailCall = (tailCallFlags & PREFIX_TAILCALL_IMPLICIT) != 0;

// Exactly one of these should be true.
assert(isExplicitTailCall != isImplicitTailCall);

// This check cannot be performed for implicit tail calls for the reason
// that impIsImplicitTailCallCandidate() is not checking whether return
// types are compatible before marking a call node with PREFIX_TAILCALL_IMPLICIT.
Expand All @@ -8378,7 +8387,7 @@ var_types Compiler::impImportCall(OPCODE opcode,
//
// For implicit tail calls, we perform this check after return types are
// known to be compatible.
if ((tailCall & PREFIX_TAILCALL_EXPLICIT) && (verCurrentState.esStackDepth != 0))
if (isExplicitTailCall && (verCurrentState.esStackDepth != 0))
{
BADCODE("Stack should be empty after tailcall");
}
Expand All @@ -8396,7 +8405,7 @@ var_types Compiler::impImportCall(OPCODE opcode,
}

// Stack empty check for implicit tail calls.
if (canTailCall && (tailCall & PREFIX_TAILCALL_IMPLICIT) && (verCurrentState.esStackDepth != 0))
if (canTailCall && isImplicitTailCall && (verCurrentState.esStackDepth != 0))
{
#ifdef TARGET_AMD64
// JIT64 Compatibility: Opportunistic tail call stack mismatch throws a VerificationException
Expand All @@ -8409,39 +8418,29 @@ var_types Compiler::impImportCall(OPCODE opcode,

// assert(compCurBB is not a catch, finally or filter block);
// assert(compCurBB is not a try block protected by a finally block);
assert(!isExplicitTailCall || compCurBB->bbJumpKind == BBJ_RETURN);

// Check for permission to tailcall
bool explicitTailCall = (tailCall & PREFIX_TAILCALL_EXPLICIT) != 0;

assert(!explicitTailCall || compCurBB->bbJumpKind == BBJ_RETURN);

// Ask VM for permission to tailcall
if (canTailCall)
{
// True virtual or indirect calls, shouldn't pass in a callee handle.
CORINFO_METHOD_HANDLE exactCalleeHnd =
((call->AsCall()->gtCallType != CT_USER_FUNC) || call->AsCall()->IsVirtual()) ? nullptr : methHnd;

if (info.compCompHnd->canTailCall(info.compMethodHnd, methHnd, exactCalleeHnd, explicitTailCall))
if (info.compCompHnd->canTailCall(info.compMethodHnd, methHnd, exactCalleeHnd, isExplicitTailCall))
{
if (explicitTailCall)
if (isExplicitTailCall)
{
// In case of explicit tail calls, mark it so that it is not considered
// for in-lining.
call->AsCall()->gtCallMoreFlags |= GTF_CALL_M_EXPLICIT_TAILCALL;
#ifdef DEBUG
if (verbose)
{
printf("\nGTF_CALL_M_EXPLICIT_TAILCALL bit set for call ");
printTreeID(call);
printf("\n");
}
#endif
JITDUMP("\nGTF_CALL_M_EXPLICIT_TAILCALL set for call [%06u]\n", dspTreeID(call));
Copy link
Contributor

@briansull briansull Mar 12, 2020

Choose a reason for hiding this comment

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

dspTreeID() really should be named getTreeID()

}
else
{
#if FEATURE_TAILCALL_OPT
// Must be an implicit tail call.
assert((tailCall & PREFIX_TAILCALL_IMPLICIT) != 0);
assert(isImplicitTailCall);

// It is possible that a call node is both an inline candidate and marked
// for opportunistic tail calling. In-lining happens before morhphing of
Expand All @@ -8450,14 +8449,7 @@ var_types Compiler::impImportCall(OPCODE opcode,
// transformed into a tail call after performing additional checks.

call->AsCall()->gtCallMoreFlags |= GTF_CALL_M_IMPLICIT_TAILCALL;
#ifdef DEBUG
if (verbose)
{
printf("\nGTF_CALL_M_IMPLICIT_TAILCALL bit set for call ");
printTreeID(call);
printf("\n");
}
#endif
JITDUMP("\nGTF_CALL_M_IMPLICIT_TAILCALL set for call [%06u]\n", dspTreeID(call));

#else //! FEATURE_TAILCALL_OPT
NYI("Implicit tail call prefix on a target which doesn't support opportunistic tail calls");
Expand All @@ -8469,39 +8461,26 @@ var_types Compiler::impImportCall(OPCODE opcode,
}
else
{
// canTailCall reported its reasons already
canTailCall = false;
// canTailCall reported its reasons already
#ifdef DEBUG
if (verbose)
{
printf("\ninfo.compCompHnd->canTailCall returned false for call ");
printTreeID(call);
printf("\n");
}
#endif
JITDUMP("\ninfo.compCompHnd->canTailCall returned false for call [%06u]\n", dspTreeID(call));
}
}
else
{
// If this assert fires it means that canTailCall was set to false without setting a reason!
assert(szCanTailCallFailReason != nullptr);

#ifdef DEBUG
if (verbose)
{
printf("\nRejecting %splicit tail call for call ", explicitTailCall ? "ex" : "im");
printTreeID(call);
printf(": %s\n", szCanTailCallFailReason);
}
#endif
info.compCompHnd->reportTailCallDecision(info.compMethodHnd, methHnd, explicitTailCall, TAILCALL_FAIL,
JITDUMP("\nRejecting %splicit tail call for [%06u]\n", isExplicitTailCall ? "ex" : "im", dspTreeID(call),
szCanTailCallFailReason);
info.compCompHnd->reportTailCallDecision(info.compMethodHnd, methHnd, isExplicitTailCall, TAILCALL_FAIL,
szCanTailCallFailReason);
}
}

// A tail recursive call is a potential loop from the current block to the start of the method.
if (canTailCall && gtIsRecursiveCall(methHnd))
if ((tailCallFlags != 0) && canTailCall && gtIsRecursiveCall(methHnd))
Copy link
Member Author

@AndyAyersMS AndyAyersMS Mar 12, 2020

Choose a reason for hiding this comment

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

This is the actual fix -- canTailCall is set to true at the top of a method, and so might still be true when we reach this point for calls that are not tail calls. So we need to also check if this call is a tail call.

{
assert(verCurrentState.esStackDepth == 0);
JITDUMP("\nFound tail recursive call in the method. Mark " FMT_BB " to " FMT_BB
" as having a backward branch.\n",
fgFirstBB->bbNum, compCurBB->bbNum);
Expand Down Expand Up @@ -8588,7 +8567,7 @@ var_types Compiler::impImportCall(OPCODE opcode,

// The CEE_READONLY prefix modifies the verification semantics of an Address
// operation on an array type.
if ((clsFlags & CORINFO_FLG_ARRAY) && readonlyCall && tiRetVal.IsByRef())
if ((clsFlags & CORINFO_FLG_ARRAY) && isReadonlyCall && tiRetVal.IsByRef())
{
tiRetVal.SetIsReadonlyByRef();
}
Expand Down