Skip to content

Commit

Permalink
JIT: Remove optLoopCompactionFixupFallThrough (#97670)
Browse files Browse the repository at this point in the history 
#97488 removed the logic for maintaining implicit fallthrough for BBJ_COND blocks into their false targets via jump insertion, but otherwise left optLoopCompactionFixupFallThrough intact. This method also tries to maintain fallthrough behavior by reversing the conditional, if the next block is the true target; per #93020, we should defer condition reversals until block reordering, so we might as well get rid of optLoopCompactionFixupFallThrough. By removing this method altogether, we don't have to worry about rebuilding the DFS tree due to the flowgraph being modified, improving TP. This removal caused relatively dramatic diffs locally, which are reduced somewhat by reversing branches (when possible) when restoring implicit fallthrough before block reordering.
  • Loading branch information
@amanasifkhalid
amanasifkhalid authored Jan 30, 2024
1 parent a74a920 commit 01bf90c
Show file tree
Hide file tree
Showing 3 changed files with 29 additions and 126 deletions.
5 changes: 2 additions & 3 deletions src/coreclr/jit/compiler.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6779,11 +6779,10 @@ class Compiler

void optFindLoops();
bool optCanonicalizeLoops();
bool optCompactLoops();
bool optCompactLoop(FlowGraphNaturalLoop* loop);
void optCompactLoops();
void optCompactLoop(FlowGraphNaturalLoop* loop);
BasicBlock* optFindLoopCompactionInsertionPoint(FlowGraphNaturalLoop* loop, BasicBlock* top);
BasicBlock* optTryAdvanceLoopCompactionInsertionPoint(FlowGraphNaturalLoop* loop, BasicBlock* insertionPoint, BasicBlock* top, BasicBlock* bottom);
bool optLoopCompactionFixupFallThrough(BasicBlock* block, BasicBlock* newNext);
bool optCreatePreheader(FlowGraphNaturalLoop* loop);
void optSetPreheaderWeight(FlowGraphNaturalLoop* loop, BasicBlock* preheader);

Expand Down
28 changes: 22 additions & 6 deletions src/coreclr/jit/fgopt.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3491,19 +3491,35 @@ bool Compiler::fgReorderBlocks(bool useProfile)
}
}

// If we will be reordering blocks, re-establish implicit fallthrough for BBJ_COND blocks
// If we will be reordering blocks, ensure the false target of a BBJ_COND block is its next block
if (useProfile)
{
for (BasicBlock* block = fgFirstBB; block != nullptr; block = block->Next())
{
if (block->KindIs(BBJ_COND) && !block->NextIs(block->GetFalseTarget()))
{
BasicBlock* jmpBlk = fgConnectFallThrough(block, block->GetFalseTarget());
assert(jmpBlk != nullptr);
assert(block->NextIs(jmpBlk));
if (block->CanRemoveJumpToTarget(block->GetTrueTarget(), this))
{
// Reverse the jump condition
GenTree* test = block->lastNode();
assert(test->OperIsConditionalJump());
test->AsOp()->gtOp1 = gtReverseCond(test->AsOp()->gtOp1);

BasicBlock* newFalseTarget = block->GetTrueTarget();
BasicBlock* newTrueTarget = block->GetFalseTarget();
block->SetTrueTarget(newTrueTarget);
block->SetFalseTarget(newFalseTarget);
assert(block->CanRemoveJumpToTarget(newFalseTarget, this));
}
else
{
BasicBlock* jmpBlk = fgConnectFallThrough(block, block->GetFalseTarget());
assert(jmpBlk != nullptr);
assert(block->NextIs(jmpBlk));

// Skip next block
block = jmpBlk;
// Skip next block
block = jmpBlk;
}
}
}
}
Expand Down
122 changes: 5 additions & 117 deletions src/coreclr/jit/optimizer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2674,12 +2674,7 @@ void Compiler::optFindLoops()
{
m_loops = FlowGraphNaturalLoops::Find(m_dfsTree);

if (optCompactLoops())
{
fgInvalidateDfsTree();
m_dfsTree = fgComputeDfs();
m_loops = FlowGraphNaturalLoops::Find(m_dfsTree);
}
optCompactLoops();

if (optCanonicalizeLoops())
{
Expand Down Expand Up @@ -2730,18 +2725,12 @@ bool Compiler::optCanonicalizeLoops()
//-----------------------------------------------------------------------------
// optCompactLoops: Compact loops to make their loop blocks lexical if possible.
//
// Returns:
// True if the flow graph was changed.
//
bool Compiler::optCompactLoops()
void Compiler::optCompactLoops()
{
bool changed = false;
for (FlowGraphNaturalLoop* loop : m_loops->InReversePostOrder())
{
changed |= optCompactLoop(loop);
optCompactLoop(loop);
}

return changed;
}

//-----------------------------------------------------------------------------
Expand All @@ -2750,18 +2739,14 @@ bool Compiler::optCompactLoops()
// Parameters:
// loop - The loop
//
// Returns:
// True if the flow graph was changed.
//
bool Compiler::optCompactLoop(FlowGraphNaturalLoop* loop)
void Compiler::optCompactLoop(FlowGraphNaturalLoop* loop)
{
BasicBlock* insertionPoint = nullptr;

BasicBlock* top = loop->GetLexicallyTopMostBlock();
unsigned numLoopBlocks = loop->NumLoopBlocks();

BasicBlock* cur = top;
bool changedFlowGraph = false;
BasicBlock* cur = top;
while (numLoopBlocks > 0)
{
if (loop->ContainsBlock(cur))
Expand Down Expand Up @@ -2816,19 +2801,11 @@ bool Compiler::optCompactLoop(FlowGraphNaturalLoop* loop)
fgMoveBlocksAfter(cur, lastNonLoopBlock, insertionPoint);
ehUpdateLastBlocks(insertionPoint, lastNonLoopBlock);

// Apply any adjustments needed for fallthrough at the boundaries of the moved region.
changedFlowGraph |= optLoopCompactionFixupFallThrough(insertionPoint, cur);
changedFlowGraph |= optLoopCompactionFixupFallThrough(lastNonLoopBlock, moveBefore);
// Also apply any adjustments needed where the blocks were snipped out of the loop.
changedFlowGraph |= optLoopCompactionFixupFallThrough(previous, nextLoopBlock);

// Update insertionPoint for the next insertion.
insertionPoint = lastNonLoopBlock;

cur = nextLoopBlock;
}

return changedFlowGraph;
}

//-----------------------------------------------------------------------------
Expand Down Expand Up @@ -2930,95 +2907,6 @@ BasicBlock* Compiler::optTryAdvanceLoopCompactionInsertionPoint(FlowGraphNatural
: newInsertionPoint;
}

//-----------------------------------------------------------------------------
// optLoopCompactionFixupFallThrough: Fix up fallthrough introduced due to
// moving a range of blocks.
//
// Parameters:
// block - Block that may have fallthrough
// newNext - The new block that was the fallthrough block
//
// Returns:
// True if the flow graph was changed by this function.
//
bool Compiler::optLoopCompactionFixupFallThrough(BasicBlock* block, BasicBlock* newNext)
{
assert(block->NextIs(newNext));
bool changed = false;

if (block->KindIs(BBJ_COND) && block->TrueTargetIs(newNext))
{
// Reverse the jump condition
GenTree* test = block->lastNode();
noway_assert(test->OperIsConditionalJump());

if (test->OperGet() == GT_JTRUE)
{
GenTree* cond = gtReverseCond(test->AsOp()->gtOp1);
assert(cond == test->AsOp()->gtOp1); // Ensure `gtReverseCond` did not create a new node.
test->AsOp()->gtOp1 = cond;
}
else
{
gtReverseCond(test);
}

// Redirect the Conditional JUMP to go to `oldNext`
block->SetTrueTarget(block->GetFalseTarget());
block->SetFalseTarget(newNext);
changed = true;
}
else if (block->KindIs(BBJ_ALWAYS) && block->TargetIs(newNext))
{
// If block is newNext's only predecessor, move the IR from block to newNext,
// but keep the now-empty block around.
//
// We move the IR because loop recognition has a very limited search capability and
// won't walk from one block's statements to another, even if the blocks form
// a linear chain. So this IR move enhances counted loop recognition.
//
// The logic here is essentially echoing fgCompactBlocks... but we don't call
// that here because we don't want to delete block and do the necessary updates
// to all the other data in flight, and we'd also prefer that newNext be the
// survivor, not block.
//
if ((newNext->bbRefs == 1) && fgCanCompactBlocks(block, newNext))
{
JITDUMP("Moving stmts from " FMT_BB " to " FMT_BB "\n", block->bbNum, newNext->bbNum);
Statement* stmtList1 = block->firstStmt();
Statement* stmtList2 = newNext->firstStmt();

// Is there anything to move?
//
if (stmtList1 != nullptr)
{
// Append newNext stmts to block's stmts.
//
if (stmtList2 != nullptr)
{
Statement* stmtLast1 = block->lastStmt();
Statement* stmtLast2 = newNext->lastStmt();

stmtLast1->SetNextStmt(stmtList2);
stmtList2->SetPrevStmt(stmtLast1);
stmtList1->SetPrevStmt(stmtLast2);
}

// Move block's stmts to newNext
//
newNext->bbStmtList = stmtList1;
block->bbStmtList = nullptr;

// Update newNext's block flags
//
newNext->CopyFlags(block, BBF_COMPACT_UPD);
}
}
}

return changed;
}

//-----------------------------------------------------------------------------
// optCreatePreheader: Create (or find) a preheader for a natural loop.
//
Expand Down

0 comments on commit 01bf90c

Please sign in to comment.