Skip to content

Commit

Permalink
JIT: build pred lists when we first build the flow graph
Browse files Browse the repository at this point in the history 
This is the last in a (long) series. We now build the pred lists at the same
time we are initially connecting up the flow graph. Pred lists are now always
valid and need to be maintained by all phases.

There are some changes needed in EH normalization, and one special case we
need to handle in debug codegen where we create the scratch BB very early on.
This was the last client for the cheap pred lists.

Note some of the pred list info can't be added right away, in particular
the "return" edges from finallies do not appear until we've made it through
the importer.

I have deferred cleaning up dead code; will do it in follow-up changes.

Contributes to dotnet#80193.
  • Loading branch information
@AndyAyersMS
AndyAyersMS committed Feb 2, 2023
1 parent bab1596 commit 6aed6b7
Show file tree
Hide file tree
Showing 3 changed files with 122 additions and 99 deletions.
11 changes: 2 additions & 9 deletions src/coreclr/jit/compiler.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3575,7 +3575,6 @@ void Compiler::compInitDebuggingInfo()

JITDUMP("Debuggable code - Add new %s to perform initialization of variables\n", fgFirstBB->dspToString());
}

/*-------------------------------------------------------------------------
*
* Read the stmt-offsets table and the line-number table
Expand Down Expand Up @@ -4349,14 +4348,8 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl
{
compFunctionTraceStart();

// Compute bbRefs and bbPreds
//
auto computePredsPhase = [this]() {
fgComputePreds();
// Enable flow graph checks
activePhaseChecks |= PhaseChecks::CHECK_FG;
};
DoPhase(this, PHASE_COMPUTE_PREDS, computePredsPhase);
// Enable flow graph checks
activePhaseChecks |= PhaseChecks::CHECK_FG;

// Prepare for importation
//
Expand Down
92 changes: 68 additions & 24 deletions src/coreclr/jit/fgbasic.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -295,7 +295,11 @@ bool Compiler::fgEnsureFirstBBisScratch()
block->bbFlags |= (BBF_INTERNAL | BBF_IMPORTED);

// This new first BB has an implicit ref, and no others.
block->bbRefs = 1;
//
// But if we call this early, before fgLinkBasicBlocks,
// defer and let it handle adding the implicit ref.
//
block->bbRefs = fgComputePredsDone ? 1 : 0;

fgFirstBBScratch = fgFirstBB;

Expand All @@ -321,7 +325,10 @@ bool Compiler::fgFirstBBisScratch()
{
assert(fgFirstBBScratch == fgFirstBB);
assert(fgFirstBBScratch->bbFlags & BBF_INTERNAL);
assert(fgFirstBBScratch->countOfInEdges() == 1);
if (fgComputePredsDone)
{
assert(fgFirstBBScratch->countOfInEdges() == 1);
}

// Normally, the first scratch block is a fall-through block. However, if the block after it was an empty
// BBJ_ALWAYS block, it might get removed, and the code that removes it will make the first scratch block
Expand Down Expand Up @@ -2717,22 +2724,42 @@ void Compiler::fgMarkBackwardJump(BasicBlock* targetBlock, BasicBlock* sourceBlo
targetBlock->bbFlags |= BBF_BACKWARD_JUMP_TARGET;
}

/*****************************************************************************
*
* Finally link up the bbJumpDest of the blocks together
*/

//------------------------------------------------------------------------
// fgLinkBasicBlocks: set block jump targets and add pred edges
//
// Notes:
// Pred edges for BBJ_EHFILTERRET are set later by fgFindBasicBlocks.
// Pred edges for BBJ_EHFINALLYRET are set later by impFixPredLists,
// after setting up the callfinally blocks.
//
void Compiler::fgLinkBasicBlocks()
{
/* Create the basic block lookup tables */

// Create the basic block lookup tables
//
fgInitBBLookup();

/* First block is always reachable */
#ifdef DEBUG
// Verify blocks are in increasing bbNum order and
// all pred list info is in initial state.
//
fgDebugCheckBBNumIncreasing();

for (BasicBlock* const block : Blocks())
{
assert(block->bbPreds == nullptr);
assert(block->bbLastPred == nullptr);
assert(block->bbRefs == 0);
}
#endif

// First block is always reachable
//
fgFirstBB->bbRefs = 1;

/* Walk all the basic blocks, filling in the target addresses */
// Special args to fgAddRefPred so it will use the initialization fast path.
//
flowList* const oldEdge = nullptr;
bool const initializingPreds = true;

for (BasicBlock* const curBBdesc : Blocks())
{
Expand All @@ -2741,15 +2768,18 @@ void Compiler::fgLinkBasicBlocks()
case BBJ_COND:
case BBJ_ALWAYS:
case BBJ_LEAVE:
curBBdesc->bbJumpDest = fgLookupBB(curBBdesc->bbJumpOffs);
curBBdesc->bbJumpDest->bbRefs++;
{
BasicBlock* const jumpDest = fgLookupBB(curBBdesc->bbJumpOffs);
curBBdesc->bbJumpDest = jumpDest;
fgAddRefPred(jumpDest, curBBdesc, oldEdge, initializingPreds);

if (curBBdesc->bbJumpDest->bbNum <= curBBdesc->bbNum)
{
fgMarkBackwardJump(curBBdesc->bbJumpDest, curBBdesc);
}

/* Is the next block reachable? */

// Is the next block reachable?
//
if (curBBdesc->KindIs(BBJ_ALWAYS, BBJ_LEAVE))
{
break;
Expand All @@ -2759,31 +2789,39 @@ void Compiler::fgLinkBasicBlocks()
{
BADCODE("Fall thru the end of a method");
}
}

// Fall through, the next block is also reachable
FALLTHROUGH;

case BBJ_NONE:
curBBdesc->bbNext->bbRefs++;
fgAddRefPred(curBBdesc->bbNext, curBBdesc, oldEdge, initializingPreds);
break;

case BBJ_EHFINALLYRET:
case BBJ_EHFILTERRET:
// We can't set up the pred list for these just yet.
// We do it in fgFindBasicBlocks.
break;

case BBJ_EHFINALLYRET:
// We can't set up the pred list for these just yet.
// We do it in impFixPredLists.
break;

case BBJ_THROW:
case BBJ_RETURN:
break;

case BBJ_SWITCH:

unsigned jumpCnt;
jumpCnt = curBBdesc->bbJumpSwt->bbsCount;
BasicBlock** jumpPtr;
jumpPtr = curBBdesc->bbJumpSwt->bbsDstTab;
{
unsigned jumpCnt = curBBdesc->bbJumpSwt->bbsCount;
BasicBlock** jumpPtr = curBBdesc->bbJumpSwt->bbsDstTab;

do
{
*jumpPtr = fgLookupBB((unsigned)*(size_t*)jumpPtr);
(*jumpPtr)->bbRefs++;
BasicBlock* jumpDest = fgLookupBB((unsigned)*(size_t*)jumpPtr);
*jumpPtr = jumpDest;
fgAddRefPred(jumpDest, curBBdesc, oldEdge, initializingPreds);
if ((*jumpPtr)->bbNum <= curBBdesc->bbNum)
{
fgMarkBackwardJump(*jumpPtr, curBBdesc);
Expand All @@ -2794,6 +2832,7 @@ void Compiler::fgLinkBasicBlocks()

noway_assert(*(jumpPtr - 1) == curBBdesc->bbNext);
break;
}

case BBJ_CALLFINALLY: // BBJ_CALLFINALLY and BBJ_EHCATCHRET don't appear until later
case BBJ_EHCATCHRET:
Expand All @@ -2802,6 +2841,10 @@ void Compiler::fgLinkBasicBlocks()
break;
}
}

// Pred lists now established.
//
fgComputePredsDone = true;
}

//------------------------------------------------------------------------
Expand Down Expand Up @@ -3609,6 +3652,7 @@ void Compiler::fgFindBasicBlocks()
{
// Mark catch handler as successor.
block->bbJumpDest = hndBegBB;
fgAddRefPred(hndBegBB, block);
assert(block->bbJumpDest->bbCatchTyp == BBCT_FILTER_HANDLER);
break;
}
Expand Down
118 changes: 52 additions & 66 deletions src/coreclr/jit/jiteh.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2131,18 +2131,10 @@ void Compiler::fgNormalizeEH()

if (modified)
{
// If we computed the cheap preds, don't let them leak out, in case other code doesn't maintain them properly.
if (fgCheapPredsValid)
{
fgRemovePreds();
}

JITDUMP("Added at least one basic block in fgNormalizeEH.\n");
fgRenumberBlocks();
#ifdef DEBUG
// fgRenumberBlocks() will dump all the blocks and the handler table, so we don't need to do it here.
fgVerifyHandlerTab();
#endif
INDEBUG(fgVerifyHandlerTab());
}
else
{
Expand Down Expand Up @@ -2182,7 +2174,16 @@ bool Compiler::fgNormalizeEHCase1()
// ...then we want to insert an empty, non-removable block outside the try to be the new first block of the
// handler.
BasicBlock* newHndStart = bbNewBasicBlock(BBJ_NONE);
fgInsertBBbefore(eh->ebdHndBeg, newHndStart);
fgInsertBBbefore(handlerStart, newHndStart);
fgAddRefPred(handlerStart, newHndStart);

// Handler begins have an extra implicit ref count.
// bbNewBasicBlock has already handled this for newHndStart.
// Remove handlerStart's implicit ref count.
//
assert(newHndStart->bbRefs == 1);
assert(handlerStart->bbRefs >= 2);
handlerStart->bbRefs--;

#ifdef DEBUG
if (verbose)
Expand Down Expand Up @@ -2238,6 +2239,7 @@ bool Compiler::fgNormalizeEHCase2()
// Note that this can only happen for nested 'try' regions, so we only need to look through the
// 'try' nesting hierarchy.
//
ArrayStack<BasicBlock*> interestingPreds(getAllocator(CMK_BasicBlock));

for (unsigned XTnum = 0; XTnum < compHndBBtabCount; XTnum++)
{
Expand Down Expand Up @@ -2337,30 +2339,28 @@ bool Compiler::fgNormalizeEHCase2()
mutualTryLast = ehOuter->ebdTryLast;
mutualProtectIndex = ehOuterTryIndex;

// We're going to need the preds. We compute them here, before inserting the new block,
// so our logic to add/remove preds below is the same for both the first time preds are
// created and subsequent times.
if (!fgCheapPredsValid)
{
fgComputeCheapPreds();
}

// We've got multiple 'try' blocks starting at the same place!
// Add a new first 'try' block for 'ehOuter' that will be outside 'eh'.

BasicBlock* newTryStart = bbNewBasicBlock(BBJ_NONE);
newTryStart->bbRefs = 0;
fgInsertBBbefore(insertBeforeBlk, newTryStart);
insertBeforeBlk->bbRefs++;
fgAddRefPred(insertBeforeBlk, newTryStart);

#ifdef DEBUG
if (verbose)
// It's possible for a try to start at the beginning of a method. If so, we need
// to adjust the implicit ref counts as we've just created a new first bb
//
if (newTryStart == fgFirstBB)
{
printf("'try' begin for EH#%u and EH#%u are same block; inserted new " FMT_BB
" before " FMT_BB " "
"as new 'try' begin for EH#%u.\n",
ehOuterTryIndex, XTnum, newTryStart->bbNum, insertBeforeBlk->bbNum, ehOuterTryIndex);
assert(insertBeforeBlk->bbRefs >= 2);
insertBeforeBlk->bbRefs--;
newTryStart->bbRefs++;
}
#endif // DEBUG

JITDUMP("'try' begin for EH#%u and EH#%u are same block; inserted new " FMT_BB " before " FMT_BB
" "
"as new 'try' begin for EH#%u.\n",
ehOuterTryIndex, XTnum, newTryStart->bbNum, insertBeforeBlk->bbNum, ehOuterTryIndex);

// The new block is the new 'try' begin.
ehOuter->ebdTryBeg = newTryStart;
Expand Down Expand Up @@ -2420,47 +2420,37 @@ bool Compiler::fgNormalizeEHCase2()
// | |----------- BB04
// |------------------ BB05

BasicBlockList* nextPred; // we're going to update the pred list as we go, so we need to keep
// track of the next pred in case it gets deleted.
for (BasicBlockList* pred = insertBeforeBlk->bbCheapPreds; pred != nullptr; pred = nextPred)
interestingPreds.Reset();
for (BasicBlock* predBlock : insertBeforeBlk->PredBlocks())
{
nextPred = pred->next;
if ((predBlock == newTryStart) || BasicBlock::sameTryRegion(insertBeforeBlk, predBlock))
{
continue;
}

// Who gets this predecessor?
BasicBlock* predBlock = pred->block;
interestingPreds.Push(predBlock);
}

if (!BasicBlock::sameTryRegion(insertBeforeBlk, predBlock))
{
// Move the edge to target newTryStart instead of insertBeforeBlk.
fgAddCheapPred(newTryStart, predBlock);
fgRemoveCheapPred(insertBeforeBlk, predBlock);

// Now change pred branches.
//
// Since cheap preds contains dups (for switch duplicates), we will call
// this once per dup.
if (predBlock->bbJumpKind != BBJ_NONE)
{
fgReplaceJumpTarget(predBlock, newTryStart, insertBeforeBlk);
}
while (interestingPreds.Height() > 0)
{
BasicBlock* const predBlock = interestingPreds.Pop();

// Need to adjust ref counts here since we're retargeting edges.
newTryStart->bbRefs++;
assert(insertBeforeBlk->countOfInEdges() > 0);
insertBeforeBlk->bbRefs--;
// Change pred branches.
//
if (predBlock->bbJumpKind != BBJ_NONE)
{
fgReplaceJumpTarget(predBlock, newTryStart, insertBeforeBlk);
}

#ifdef DEBUG
if (verbose)
{
printf("Redirect " FMT_BB " target from " FMT_BB " to " FMT_BB ".\n",
predBlock->bbNum, insertBeforeBlk->bbNum, newTryStart->bbNum);
}
#endif // DEBUG
if ((predBlock->bbNext == newTryStart) && predBlock->bbFallsThrough())
{
fgRemoveRefPred(insertBeforeBlk, predBlock);
fgAddRefPred(newTryStart, predBlock);
}
}

// The new block (a fall-through block) is a new predecessor.
fgAddCheapPred(insertBeforeBlk, newTryStart);
JITDUMP("Redirect " FMT_BB " target from " FMT_BB " to " FMT_BB ".\n", predBlock->bbNum,
insertBeforeBlk->bbNum, newTryStart->bbNum);
}

// We don't need to update the tryBeg block of other EH regions here because we are looping
// outwards in enclosing try index order, and we'll get to them later.
Expand Down Expand Up @@ -2833,6 +2823,7 @@ bool Compiler::fgNormalizeEHCase3()
// shares a 'last' pointer

BasicBlock* newLast = bbNewBasicBlock(BBJ_NONE);
newLast->bbRefs = 0;
assert(insertAfterBlk != nullptr);
fgInsertBBafter(insertAfterBlk, newLast);

Expand Down Expand Up @@ -2880,12 +2871,7 @@ bool Compiler::fgNormalizeEHCase3()
newLast->bbCodeOffsEnd = newLast->bbCodeOffs; // code size = 0. TODO: use BAD_IL_OFFSET instead?
newLast->inheritWeight(insertAfterBlk);
newLast->bbFlags |= BBF_INTERNAL;

// The new block (a fall-through block) is a new predecessor.
if (fgCheapPredsValid)
{
fgAddCheapPred(newLast, insertAfterBlk);
}
fgAddRefPred(newLast, insertAfterBlk);

// Move the insert pointer. More enclosing equivalent 'last' blocks will be inserted after this.
insertAfterBlk = newLast;
Expand Down

0 comments on commit 6aed6b7

Please sign in to comment.