Skip to content

Commit

Permalink
JIT: add missing recursive tail call detection logic (#93892)
Browse files Browse the repository at this point in the history
We now require that recursive tail calls be noted before morph by setting
method and block flags. Add missing flag setting when devirtualization creates
a recursive tail call candidate.

Also add a diagnosic post-phase check to catch cases like this earlier.

Fixes #93844.
  • Loading branch information
AndyAyersMS authored Oct 23, 2023
1 parent 4c71cfb commit 87c2558
Show file tree
Hide file tree
Showing 4 changed files with 32 additions and 12 deletions.
4 changes: 3 additions & 1 deletion src/coreclr/jit/compiler.h
Original file line number Diff line number Diff line change
Expand Up @@ -4703,6 +4703,8 @@ class Compiler
bool fgGlobalMorph; // indicates if we are during the global morphing phase
// since fgMorphTree can be called from several places

bool fgGlobalMorphDone;

bool impBoxTempInUse; // the temp below is valid and available
unsigned impBoxTemp; // a temporary that is used for boxing

Expand Down Expand Up @@ -5663,7 +5665,7 @@ class Compiler
void fgDebugCheckLoopTable();
void fgDebugCheckSsa();

void fgDebugCheckFlags(GenTree* tree);
void fgDebugCheckFlags(GenTree* tree, BasicBlock* block);
void fgDebugCheckDispFlags(GenTree* tree, GenTreeFlags dispFlags, GenTreeDebugFlags debugFlags);
void fgDebugCheckFlagsHelper(GenTree* tree, GenTreeFlags actualFlags, GenTreeFlags expectedFlags);
void fgDebugCheckTryFinallyExits();
Expand Down
6 changes: 4 additions & 2 deletions src/coreclr/jit/fgbasic.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,10 @@ void Compiler::fgInit()
genReturnLocal = BAD_VAR_NUM;

/* We haven't reached the global morphing phase */
fgGlobalMorph = false;
fgModified = false;
fgGlobalMorph = false;
fgGlobalMorphDone = false;

fgModified = false;

#ifdef DEBUG
fgSafeBasicBlockCreation = true;
Expand Down
26 changes: 17 additions & 9 deletions src/coreclr/jit/fgdiagnostic.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3153,12 +3153,13 @@ void Compiler::fgDebugCheckBBlist(bool checkBBNum /* = false */, bool checkBBRef

//------------------------------------------------------------------------
// fgDebugCheckFlags: Validate various invariants related to the propagation
// and setting of tree flags ("gtFlags").
// and setting of tree, block, and method flags
//
// Arguments:
// tree - the tree to (recursively) check the flags for
// block - basic block containing the tree
//
void Compiler::fgDebugCheckFlags(GenTree* tree)
void Compiler::fgDebugCheckFlags(GenTree* tree, BasicBlock* block)
{
GenTreeFlags actualFlags = tree->gtFlags & GTF_ALL_EFFECT;
GenTreeFlags expectedFlags = GTF_EMPTY;
Expand Down Expand Up @@ -3245,10 +3246,17 @@ void Compiler::fgDebugCheckFlags(GenTree* tree)
break;

case GT_CALL:
{
GenTreeCall* const call = tree->AsCall();

GenTreeCall* call;

call = tree->AsCall();
// Before global morph, if there are recursive tail calls, we should have
// set the associated block and method flags.
//
if (!fgGlobalMorphDone && call->CanTailCall() && gtIsRecursiveCall(call))
{
assert(doesMethodHaveRecursiveTailcall());
assert((block->bbFlags & BBF_RECURSIVE_TAILCALL) != 0);
}

for (CallArg& arg : call->gtArgs.Args())
{
Expand All @@ -3264,8 +3272,8 @@ void Compiler::fgDebugCheckFlags(GenTree* tree)
actualFlags |= arg.GetLateNode()->gtFlags & GTF_ASG;
}
}

break;
}
break;

case GT_CMPXCHG:
expectedFlags |= (GTF_GLOB_REF | GTF_ASG);
Expand Down Expand Up @@ -3341,7 +3349,7 @@ void Compiler::fgDebugCheckFlags(GenTree* tree)
}

tree->VisitOperands([&](GenTree* operand) -> GenTree::VisitResult {
fgDebugCheckFlags(operand);
fgDebugCheckFlags(operand, block);
expectedFlags |= (operand->gtFlags & GTF_ALL_EFFECT);

return GenTree::VisitResult::Continue;
Expand Down Expand Up @@ -3749,7 +3757,7 @@ void Compiler::fgDebugCheckStmtsList(BasicBlock* block, bool morphTrees)
gtDispTree(stmt->GetRootNode());
}

fgDebugCheckFlags(stmt->GetRootNode());
fgDebugCheckFlags(stmt->GetRootNode(), block);

// Not only will this stress fgMorphBlockStmt(), but we also get all the checks
// done by fgMorphTree()
Expand Down
8 changes: 8 additions & 0 deletions src/coreclr/jit/importercalls.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7412,6 +7412,14 @@ void Compiler::impDevirtualizeCall(GenTreeCall* call,
*pExactContextHandle = MAKE_CLASSCONTEXT(derivedClass);
}

// We might have created a new recursive tail call candidate.
//
if (call->CanTailCall() && gtIsRecursiveCall(derivedMethod))
{
setMethodHasRecursiveTailcall();
compCurBB->bbFlags |= BBF_RECURSIVE_TAILCALL;
}

#ifdef FEATURE_READYTORUN
if (opts.IsReadyToRun())
{
Expand Down

0 comments on commit 87c2558

Please sign in to comment.