From 7113338ea116d796b555583583a21bbb38c750b6 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Fri, 5 Jan 2024 10:39:29 +0100 Subject: [PATCH] JIT: Remove loop unrolling quirks (#96477) Minor diffs expected due to no longer updating `BasicBlock::bbNatLoopNum`, and `fgCanCompactBlocks` having checks on this field. --- src/coreclr/jit/optimizer.cpp | 64 ++++++++--------------------------- 1 file changed, 14 insertions(+), 50 deletions(-) diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index 37113605b5f65..394b822e98da9 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -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; } @@ -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 @@ -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 @@ -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()"); @@ -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 @@ -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 @@ -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 @@ -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; } @@ -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); @@ -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;