Skip to content

Commit

Permalink
JIT: use bbID for pred list ordering (dotnet#108357)
Browse files Browse the repository at this point in the history 
Make `bbID` available in release builds, and use it to control the order
of edges in the pred list. This removes (almost all) the need for reordering
pred lists when blocks are renumbered (`bbNum` changes, but `bbID` doesn't).

Also remove the need for sorting pred lists; in the one remaining place
where sorting might be needed, just remove, modify, and then re-add the edge.
  • Loading branch information
@AndyAyersMS @sirntar
AndyAyersMS authored and sirntar committed Oct 3, 2024
1 parent 1ce3747 commit deceea4
Show file tree
Hide file tree
Showing 6 changed files with 43 additions and 152 deletions.
125 changes: 11 additions & 114 deletions src/coreclr/jit/block.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -390,132 +390,32 @@ bool BasicBlock::CanRemoveJumpToTarget(BasicBlock* target, Compiler* compiler) c
return NextIs(target) && !compiler->fgInDifferentRegions(this, target);
}

#ifdef DEBUG

//------------------------------------------------------------------------
// checkPredListOrder: see if pred list is properly ordered
//
// Returns:
// false if pred list is not in increasing bbNum order.
// false if pred list is not in increasing bbID order.
//
bool BasicBlock::checkPredListOrder()
{
unsigned lastBBNum = 0;
unsigned lastBBID = 0;
bool compare = false;
for (BasicBlock* const predBlock : PredBlocks())
{
const unsigned bbNum = predBlock->bbNum;
if (bbNum <= lastBBNum)
const unsigned bbID = predBlock->bbID;
if (compare && (bbID <= lastBBID))
{
assert(bbNum != lastBBNum);
assert(bbID != lastBBID);
return false;
}
lastBBNum = bbNum;
compare = true;
lastBBID = bbID;
}
return true;
}

//------------------------------------------------------------------------
// ensurePredListOrder: ensure all pred list entries appear in increasing
// bbNum order.
//
// Arguments:
// compiler - current compiler instance
//
void BasicBlock::ensurePredListOrder(Compiler* compiler)
{
// First, check if list is already in order.
//
if (checkPredListOrder())
{
return;
}

reorderPredList(compiler);
assert(checkPredListOrder());
}

//------------------------------------------------------------------------
// reorderPredList: relink pred list in increasing bbNum order.
//
// Arguments:
// compiler - current compiler instance
//
void BasicBlock::reorderPredList(Compiler* compiler)
{
// Count number or entries.
//
int count = 0;
for (FlowEdge* const pred : PredEdges())
{
count++;
}

// If only 0 or 1 entry, nothing to reorder.
//
if (count < 2)
{
return;
}

// Allocate sort vector if needed.
//
if (compiler->fgPredListSortVector == nullptr)
{
CompAllocator allocator = compiler->getAllocator(CMK_FlowEdge);
compiler->fgPredListSortVector = new (allocator) jitstd::vector<FlowEdge*>(allocator);
}

jitstd::vector<FlowEdge*>* const sortVector = compiler->fgPredListSortVector;
sortVector->clear();

// Fill in the vector from the list.
//
for (FlowEdge* const pred : PredEdges())
{
sortVector->push_back(pred);
}

// Sort by increasing bbNum
//
struct FlowEdgeBBNumCmp
{
bool operator()(const FlowEdge* f1, const FlowEdge* f2)
{
return f1->getSourceBlock()->bbNum < f2->getSourceBlock()->bbNum;
}
};

jitstd::sort(sortVector->begin(), sortVector->end(), FlowEdgeBBNumCmp());

// Rethread the list.
//
FlowEdge* last = nullptr;

for (FlowEdge* current : *sortVector)
{
if (last == nullptr)
{
bbPreds = current;
}
else
{
last->setNextPredEdge(current);
}

last = current;
}

last->setNextPredEdge(nullptr);

// Note bbLastPred is only used transiently, during
// initial pred list construction.
//
if (!compiler->fgPredsComputed)
{
bbLastPred = last;
}
}

#ifdef DEBUG

//------------------------------------------------------------------------
// dspBlockILRange(): Display the block's IL range as [XXX...YYY), where XXX and YYY might be "???" for BAD_IL_OFFSET.
//
Expand Down Expand Up @@ -1631,10 +1531,7 @@ BasicBlock* BasicBlock::New(Compiler* compiler)
// boundaries), or have been inserted by the JIT
block->bbCodeOffs = BAD_IL_OFFSET;
block->bbCodeOffsEnd = BAD_IL_OFFSET;

#ifdef DEBUG
block->bbID = compiler->compBasicBlockID++;
#endif
block->bbID = compiler->compBasicBlockID++;

/* Give the block a number, set the ancestor count and weight */

Expand Down
9 changes: 4 additions & 5 deletions src/coreclr/jit/block.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1537,11 +1537,9 @@ struct BasicBlock : private LIR::Range
return PredBlockList<true>(bbPreds);
}

// Pred list maintenance
//
#ifdef DEBUG
bool checkPredListOrder();
void ensurePredListOrder(Compiler* compiler);
void reorderPredList(Compiler* compiler);
#endif

union
{
Expand Down Expand Up @@ -1672,9 +1670,10 @@ struct BasicBlock : private LIR::Range
// still in the BB list by whether they have the same stamp (with high probability).
unsigned bbTraversalStamp;

#endif // DEBUG

// bbID is a unique block identifier number that does not change: it does not get renumbered, like bbNum.
unsigned bbID;
#endif // DEBUG

unsigned bbStackDepthOnEntry() const;
void bbSetStack(StackEntry* stack);
Expand Down
12 changes: 7 additions & 5 deletions src/coreclr/jit/compiler.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7018,12 +7018,10 @@ int Compiler::compCompileHelper(CORINFO_MODULE_HANDLE classPtr,
}
}

#ifdef DEBUG
if (compIsForInlining())
{
compBasicBlockID = impInlineInfo->InlinerCompiler->compBasicBlockID;
}
#endif

const bool forceInline = !!(info.compFlags & CORINFO_FLG_FORCEINLINE);

Expand Down Expand Up @@ -7250,12 +7248,16 @@ int Compiler::compCompileHelper(CORINFO_MODULE_HANDLE classPtr,
#ifdef DEBUG
if (compIsForInlining())
{
impInlineInfo->InlinerCompiler->compGenTreeID = compGenTreeID;
impInlineInfo->InlinerCompiler->compStatementID = compStatementID;
impInlineInfo->InlinerCompiler->compBasicBlockID = compBasicBlockID;
impInlineInfo->InlinerCompiler->compGenTreeID = compGenTreeID;
impInlineInfo->InlinerCompiler->compStatementID = compStatementID;
}
#endif

if (compIsForInlining())
{
impInlineInfo->InlinerCompiler->compBasicBlockID = compBasicBlockID;
}

_Next:

if (compDonotInline())
Expand Down
8 changes: 4 additions & 4 deletions src/coreclr/jit/compiler.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10873,11 +10873,11 @@ class Compiler

public:
#ifdef DEBUG
unsigned compGenTreeID = 0;
unsigned compStatementID = 0;
unsigned compBasicBlockID = 0;
unsigned compGenTreeID = 0;
unsigned compStatementID = 0;
#endif
LONG compMethodID = 0;
unsigned compBasicBlockID = 0;
LONG compMethodID = 0;

BasicBlock* compCurBB = nullptr; // the current basic block in process
Statement* compCurStmt; // the current statement in process
Expand Down
23 changes: 12 additions & 11 deletions src/coreclr/jit/fgbasic.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -632,21 +632,26 @@ void Compiler::fgReplaceJumpTarget(BasicBlock* block, BasicBlock* oldTarget, Bas
// Note that the successor block's bbRefs is not changed, since it has the same number of
// references as before, just from a different predecessor block.
//
// Also note this may cause sorting of the pred list.
// Also note this may cause reordering of the pred list.
//
void Compiler::fgReplacePred(FlowEdge* edge, BasicBlock* const newPred)
{
assert(edge != nullptr);
assert(newPred != nullptr);
assert(edge->getSourceBlock() != newPred);

edge->setSourceBlock(newPred);

// We may now need to reorder the pred list.
// Remove the edge, modify it, then add it back
//
BasicBlock* succBlock = edge->getDestinationBlock();
assert(succBlock != nullptr);
succBlock->ensurePredListOrder(this);
BasicBlock* const target = edge->getDestinationBlock();
BasicBlock* const oldPred = edge->getSourceBlock();
FlowEdge** listp = fgGetPredInsertPoint(oldPred, target);
assert(*listp == edge);
*listp = edge->getNextPredEdge();
edge->setSourceBlock(newPred);
listp = fgGetPredInsertPoint(newPred, target);
edge->setNextPredEdge(*listp);
*listp = edge;
assert(target->checkPredListOrder());
}

/*****************************************************************************
Expand Down Expand Up @@ -5407,10 +5412,6 @@ bool Compiler::fgRenumberBlocks()
//
if (renumbered)
{
for (BasicBlock* const block : Blocks())
{
block->ensurePredListOrder(this);
}
JITDUMP("\n*************** After renumbering the basic blocks\n");
JITDUMPEXEC(fgDispBasicBlocks());
JITDUMPEXEC(fgDispHandlerTab());
Expand Down
18 changes: 5 additions & 13 deletions src/coreclr/jit/fgflow.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,26 +104,18 @@ FlowEdge* Compiler::fgAddRefPred(BasicBlock* block, BasicBlock* blockPred, FlowE

block->bbRefs++;

// Keep the predecessor list in lowest to highest bbNum order. This allows us to discover the loops in
// optFindNaturalLoops from innermost to outermost.
// Keep the predecessor list in lowest to highest bbID order.
//
// If we are initializing preds, we rely on the fact that we are adding references in increasing
// order of blockPred->bbNum to avoid searching the list.
//
// TODO-Throughput: Inserting an edge for a block in sorted order requires searching every existing edge.
// Thus, inserting all the edges for a block is quadratic in the number of edges. We need to either
// not bother sorting for debuggable code, or sort in optFindNaturalLoops, or better, make the code in
// optFindNaturalLoops not depend on order. This also requires ensuring that nobody else has taken a
// dependency on this order. Note also that we don't allow duplicates in the list; we maintain a DupCount
// count of duplication. This also necessitates walking the flow list for every edge we add.
// order of blockPred->bbID to avoid searching the list.
//
FlowEdge* flow = nullptr;
FlowEdge** listp;

if (initializingPreds)
{
// List is sorted order and we're adding references in
// increasing blockPred->bbNum order. The only possible
// increasing blockPred->bbID order. The only possible
// dup list entry is the last one.
//
listp = &block->bbPreds;
Expand All @@ -132,7 +124,7 @@ FlowEdge* Compiler::fgAddRefPred(BasicBlock* block, BasicBlock* blockPred, FlowE
{
listp = flowLast->getNextPredEdgeRef();

assert(flowLast->getSourceBlock()->bbNum <= blockPred->bbNum);
assert(flowLast->getSourceBlock()->bbID <= blockPred->bbID);

if (flowLast->getSourceBlock() == blockPred)
{
Expand Down Expand Up @@ -365,7 +357,7 @@ FlowEdge** Compiler::fgGetPredInsertPoint(BasicBlock* blockPred, BasicBlock* new

// Search pred list for insertion point
//
while ((*listp != nullptr) && ((*listp)->getSourceBlock()->bbNum < blockPred->bbNum))
while ((*listp != nullptr) && ((*listp)->getSourceBlock()->bbID < blockPred->bbID))
{
listp = (*listp)->getNextPredEdgeRef();
}
Expand Down

0 comments on commit deceea4

Please sign in to comment.