Skip to content

Commit

Permalink
JIT: Remove loop unrolling quirks (#96477)
Browse files Browse the repository at this point in the history 
Minor diffs expected due to no longer updating
`BasicBlock::bbNatLoopNum`, and `fgCanCompactBlocks` having checks on
this field.
  • Loading branch information
@jakobbotsch
jakobbotsch authored Jan 5, 2024
1 parent 09b925d commit 7113338
Showing 1 changed file with 14 additions and 50 deletions.
64 changes: 14 additions & 50 deletions src/coreclr/jit/optimizer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4010,13 +4010,8 @@ PhaseStatus Compiler::optUnrollLoops()
// Visit loops in post order (inner loops before outer loops).
for (FlowGraphNaturalLoop* loop : m_loops->InPostOrder())
{
LoopDsc* oldLoop = m_newToOldLoop[loop->GetIndex()];
// TODO-Quirk: Remove
if (oldLoop == nullptr)
{
continue;
}
if ((oldLoop->lpFlags & (LPFLG_DONT_UNROLL | LPFLG_REMOVED)) != 0)
if (m_newToOldLoop[loop->GetIndex()] == nullptr)
{
continue;
}
Expand All @@ -4028,19 +4023,10 @@ PhaseStatus Compiler::optUnrollLoops()

NaturalLoopIterInfo iterInfo;
if (!loop->AnalyzeIteration(&iterInfo))
{
assert((oldLoop->lpFlags & LPFLG_ITER) == 0);
continue;
}

// TODO-Quirk: Allow this
if ((oldLoop->lpFlags & LPFLG_ITER) == 0)
{
continue;
}

optCrossCheckIterInfo(iterInfo, *oldLoop);

// Check for required flags:
// HasConstInit - required because this transform only handles full unrolls
// HasConstLimit - required because this transform only handles full unrolls
Expand All @@ -4050,13 +4036,6 @@ PhaseStatus Compiler::optUnrollLoops()
continue;
}

// TODO-Quirk: Unnecessary
if (!oldLoop->lpIsTopEntry())
{
JITDUMP("Failed to unroll loop " FMT_LP ": not top entry\n", loop->GetIndex());
continue;
}

// Get the loop data:
// - initial constant
// - limit constant
Expand Down Expand Up @@ -4165,7 +4144,6 @@ PhaseStatus Compiler::optUnrollLoops()
(incr->AsOp()->gtOp2->gtOper != GT_CNS_INT) ||
(incr->AsOp()->gtOp2->AsIntCon()->gtIconVal != iterInc) ||

// TODO-Quirk: This check shouldn't be needed.
(iterInfo.TestBlock->lastStmt()->GetRootNode()->gtGetOp1() != iterInfo.TestTree))
{
noway_assert(!"Bad precondition in Compiler::optUnrollLoops()");
Expand Down Expand Up @@ -4273,12 +4251,11 @@ PhaseStatus Compiler::optUnrollLoops()

BlockToBlockMap blockMap(getAllocator(CMK_LoopUnroll));

BasicBlock* bottom = loop->GetLexicallyBottomMostBlock();
BasicBlock* insertAfter = bottom;
BasicBlock* const tail = bottom->Next();
BasicBlock* prevTestBlock = nullptr;
BasicBlock::loopNumber newLoopNum = oldLoop->lpParent;
unsigned iterToUnroll = totalIter; // The number of iterations left to unroll
BasicBlock* bottom = loop->GetLexicallyBottomMostBlock();
BasicBlock* insertAfter = bottom;
BasicBlock* const tail = bottom->Next();
BasicBlock* prevTestBlock = nullptr;
unsigned iterToUnroll = totalIter; // The number of iterations left to unroll

// Find the exit block of the IV test first. We need to do that
// here since it may have implicit fallthrough that we'll change
Expand All @@ -4289,8 +4266,9 @@ PhaseStatus Compiler::optUnrollLoops()
BasicBlock* exit =
loop->ContainsBlock(exiting->GetTrueTarget()) ? exiting->GetFalseTarget() : exiting->GetTrueTarget();

// If the bottom block falls out of the loop, then insert explicit block to branch around unrolled
// iterations.
// If the bottom block falls out of the loop, then insert an
// explicit block to branch around the unrolled iterations we are
// going to create.
if (bottom->KindIs(BBJ_COND))
{
// TODO-NoFallThrough: Shouldn't need new BBJ_ALWAYS block once bbFalseTarget can diverge from bbNext
Expand Down Expand Up @@ -4329,22 +4307,12 @@ PhaseStatus Compiler::optUnrollLoops()
// put the loop blocks back to how they were before we started cloning blocks,
// and abort unrolling the loop.
bottom->SetNext(tail);
oldLoop->lpFlags |= LPFLG_DONT_UNROLL; // Mark it so we don't try to unroll it again.
INDEBUG(++unrollFailures);
JITDUMP("Failed to unroll loop " FMT_LP ": block cloning failed on " FMT_BB "\n",
loop->GetIndex(), block->bbNum);
return BasicBlockVisit::Abort;
}

// All blocks in the unrolled loop will now be marked with the parent loop number. Note that
// if the loop being unrolled contains nested (child) loops, we will notice this below (when
// we set anyNestedLoopsUnrolledThisLoop), and that will cause us to rebuild the entire loop
// table and all loop annotations on blocks. However, if the loop contains no nested loops,
// setting the block `bbNatLoopNum` here is sufficient to incrementally update the block's
// loop info.

newBlock->bbNatLoopNum = newLoopNum;

// Block weight should no longer have the loop multiplier
//
// Note this is not quite right, as we may not have upscaled by this amount
Expand Down Expand Up @@ -4374,9 +4342,10 @@ PhaseStatus Compiler::optUnrollLoops()
testCopyStmt->SetRootNode(sideEffList);
}

// Save the block so we can special case it when
// redirecting the blocks below; we don't want this one
// to become BBJ_COND, it should stay as BBJ_ALWAYS.
// Save the test block of the previously unrolled
// iteration, so that we can redirect it when we create
// the next iteration (or to the exit for the last
// iteration).
assert(testBlock == nullptr);
testBlock = newBlock;
}
Expand All @@ -4392,8 +4361,7 @@ PhaseStatus Compiler::optUnrollLoops()
BasicBlock* newRedirBlk =
fgNewBBafter(BBJ_ALWAYS, insertAfter, /* extendRegion */ true, targetBlk);
newRedirBlk->copyEHRegion(insertAfter);
newRedirBlk->bbNatLoopNum = newLoopNum;
newRedirBlk->bbWeight = block->Next()->bbWeight;
newRedirBlk->bbWeight = block->Next()->bbWeight;
newRedirBlk->CopyFlags(block->Next(), BBF_RUN_RARELY | BBF_PROF_WEIGHT);
newRedirBlk->scaleBBWeight(1.0 / BB_LOOP_WEIGHT_SCALE);

Expand Down Expand Up @@ -4550,10 +4518,6 @@ PhaseStatus Compiler::optUnrollLoops()
}
#endif // DEBUG

// TODO: Remove
// Update loop table.
optMarkLoopRemoved((unsigned)(oldLoop - optLoopTable));

// Remember that something has changed.
INDEBUG(++unrollCount);
change = true;
Expand Down

0 comments on commit 7113338

Please sign in to comment.