JIT: Invalidate loop table after unrolling and dominators after MarkLocalVars

src/coreclr/jit/block.cpp

@@ -591,6 +591,10 @@ void BasicBlock::dspFlags()
     {
         printf("gcpoll ");
     }
+    if (HasFlag(BBF_OLD_LOOP_HEADER_QUIRK))
+    {
+        printf("loopheader ");
+    }
 }
 
 /*****************************************************************************

src/coreclr/jit/compiler.cpp

@@ -4833,9 +4833,9 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl
         //
         DoPhase(this, PHASE_UNROLL_LOOPS, &Compiler::optUnrollLoops);
 
-        // Clear loop table info that is not used after this point, and might become invalid.
-        //
-        DoPhase(this, PHASE_CLEAR_LOOP_INFO, &Compiler::optClearLoopIterInfo);
+        // The loop table is no longer valid.
+        optLoopTableValid = false;
+        optLoopTable      = nullptr;
     }
 
 #ifdef DEBUG
@@ -4852,6 +4852,10 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl
     //
     DoPhase(this, PHASE_MARK_LOCAL_VARS, &Compiler::lvaMarkLocalVars);
 
+    // Dominator and reachability sets are no longer valid.
+    fgDomsComputed         = false;
+    fgCompactRenumberQuirk = true;
+
     // IMPORTANT, after this point, locals are ref counted.
     // However, ref counts are not kept incrementally up to date.
     assert(lvaLocalVarRefCounted());
@@ -5023,11 +5027,8 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl
         }
     }
 
-    // Dominator and reachability sets are no longer valid.
-    // The loop table is no longer valid.
-    fgDomsComputed            = false;
-    optLoopTableValid         = false;
     optLoopsRequirePreHeaders = false;
+    fgCompactRenumberQuirk    = false;
 
 #ifdef DEBUG
     DoPhase(this, PHASE_STRESS_SPLIT_TREE, &Compiler::StressSplitTree);
@@ -5897,13 +5898,17 @@ void Compiler::RecomputeLoopInfo()
     fgDomsComputed = false;
     fgComputeReachability();
     optSetBlockWeights();
-    // Rebuild the loop tree annotations themselves
-    // But don't leave the iter info lying around.
     optFindLoops();
-    optClearLoopIterInfo();
 
     m_dfsTree = fgComputeDfs();
     optFindNewLoops();
+
+    // Dominators and the loop table are computed above for old<->new loop
+    // crossreferencing, but they are not actually used for optimization
+    // anymore.
+    optLoopTableValid = false;
+    optLoopTable      = nullptr;
+    fgDomsComputed    = false;
 }
 
 /*****************************************************************************/

src/coreclr/jit/compiler.h

@@ -5102,7 +5102,8 @@ class Compiler
     bool fgDomsComputed;         // Have we computed the dominator sets?
     bool fgReturnBlocksComputed; // Have we computed the return blocks list?
     bool fgOptimizedFinally;     // Did we optimize any try-finallys?
-    bool fgCanonicalizedFirstBB; // Quirk: did we end up canonicalizing first BB?
+    bool fgCanonicalizedFirstBB; // TODO-Quirk: did we end up canonicalizing first BB?
+    bool fgCompactRenumberQuirk; // TODO-Quirk: Should fgCompactBlocks renumber BBs above fgDomBBcount?
 
     bool fgHasSwitch; // any BBJ_SWITCH jumps?
 
@@ -6800,10 +6801,6 @@ class Compiler
     //   VNPhi's connect VN's to the SSA definition, so we can know if the SSA def occurs in the loop.
     bool optVNIsLoopInvariant(ValueNum vn, FlowGraphNaturalLoop* loop, VNSet* recordedVNs);
 
-    // If "blk" is the entry block of a natural loop, returns true and sets "*pLnum" to the index of the loop
-    // in the loop table.
-    bool optBlockIsLoopEntry(BasicBlock* blk, unsigned* pLnum);
-
     // Records the set of "side effects" of all loops: fields (object instance and static)
     // written to, and SZ-array element type equivalence classes updated.
     void optComputeLoopSideEffects();
@@ -7106,8 +7103,6 @@ class Compiler
                        BasicBlock*   exit,
                        unsigned char exitCnt);
 
-    PhaseStatus optClearLoopIterInfo();
-
 #ifdef DEBUG
     void optPrintLoopInfo(unsigned lnum, bool printVerbose = false);
     void optPrintLoopInfo(const LoopDsc* loop, bool printVerbose = false);

src/coreclr/jit/compphases.h

@@ -70,7 +70,6 @@ CompPhaseNameMacro(PHASE_ZERO_INITS,                 "Redundant zero Inits",
 CompPhaseNameMacro(PHASE_FIND_LOOPS,                 "Find loops",                     false, -1, false)
 CompPhaseNameMacro(PHASE_CLONE_LOOPS,                "Clone loops",                    false, -1, false)
 CompPhaseNameMacro(PHASE_UNROLL_LOOPS,               "Unroll loops",                   false, -1, false)
-CompPhaseNameMacro(PHASE_CLEAR_LOOP_INFO,            "Clear loop info",                false, -1, false)
 CompPhaseNameMacro(PHASE_MORPH_MDARR,                "Morph array ops",                false, -1, false)
 CompPhaseNameMacro(PHASE_HOIST_LOOP_CODE,            "Hoist loop code",                false, -1, false)
 CompPhaseNameMacro(PHASE_MARK_LOCAL_VARS,            "Mark local vars",                false, -1, false)

src/coreclr/jit/fgbasic.cpp

@@ -33,6 +33,7 @@ void Compiler::fgInit()
     /* We haven't yet computed the dominator sets */
     fgDomsComputed         = false;
     fgReturnBlocksComputed = false;
+    fgCompactRenumberQuirk = false;
 
 #ifdef DEBUG
     fgReachabilitySetsValid = false;

src/coreclr/jit/fgdiagnostic.cpp

@@ -4763,7 +4763,6 @@ void Compiler::fgDebugCheckSsa()
     }
 
     assert(fgSsaPassesCompleted > 0);
-    assert(fgDomsComputed);
 
     // Visit the blocks that SSA initially renamed
     //

src/coreclr/jit/fgopt.cpp

@@ -1925,7 +1925,7 @@ bool Compiler::fgCanCompactBlocks(BasicBlock* block, BasicBlock* bNext)
     }
 
     // Don't compact away any loop entry blocks that we added in optCanonicalizeLoops
-    if (optIsLoopEntry(block))
+    if (block->HasFlag(BBF_OLD_LOOP_HEADER_QUIRK))
     {
         return false;
     }
@@ -2355,20 +2355,28 @@ void Compiler::fgCompactBlocks(BasicBlock* block, BasicBlock* bNext)
     // before the updates, we can create pred lists with duplicate m_block->bbNum
     // values (though different m_blocks).
     //
-    if (fgDomsComputed && (block->bbNum > fgDomBBcount))
+    if ((fgDomsComputed || fgCompactRenumberQuirk) && (block->bbNum > fgDomBBcount))
     {
-        assert(fgReachabilitySetsValid);
-        BlockSetOps::Assign(this, block->bbReach, bNext->bbReach);
-        BlockSetOps::ClearD(this, bNext->bbReach);
+        if (fgDomsComputed)
+        {
+            assert(fgReachabilitySetsValid);
+            BlockSetOps::Assign(this, block->bbReach, bNext->bbReach);
+            BlockSetOps::ClearD(this, bNext->bbReach);
 
-        block->bbIDom = bNext->bbIDom;
-        bNext->bbIDom = nullptr;
+            block->bbIDom = bNext->bbIDom;
+            bNext->bbIDom = nullptr;
 
-        // In this case, there's no need to update the preorder and postorder numbering
-        // since we're changing the bbNum, this makes the basic block all set.
-        //
-        JITDUMP("Renumbering " FMT_BB " to be " FMT_BB " to preserve dominator information\n", block->bbNum,
-                bNext->bbNum);
+            // In this case, there's no need to update the preorder and postorder numbering
+            // since we're changing the bbNum, this makes the basic block all set.
+            //
+            JITDUMP("Renumbering " FMT_BB " to be " FMT_BB " to preserve dominator information\n", block->bbNum,
+                    bNext->bbNum);
+        }
+        else
+        {
+            // TODO-Quirk: Remove
+            JITDUMP("Renumbering " FMT_BB " to be " FMT_BB " for a quirk\n", block->bbNum, bNext->bbNum);
+        }
 
         block->bbNum = bNext->bbNum;
 
@@ -2387,7 +2395,10 @@ void Compiler::fgCompactBlocks(BasicBlock* block, BasicBlock* bNext)
         }
     }
 
-    fgUpdateLoopsAfterCompacting(block, bNext);
+    if (optLoopTableValid)
+    {
+        fgUpdateLoopsAfterCompacting(block, bNext);
+    }
 
 #if DEBUG
     if (verbose && 0)
@@ -6227,8 +6238,11 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication, bool isPhase)
                         /* Mark the block as removed */
                         bNext->SetFlags(BBF_REMOVED);
 
-                        // Update the loop table if we removed the bottom of a loop, for example.
-                        fgUpdateLoopsAfterCompacting(block, bNext);
+                        if (optLoopTableValid)
+                        {
+                            // Update the loop table if we removed the bottom of a loop, for example.
+                            fgUpdateLoopsAfterCompacting(block, bNext);
+                        }
 
                         // If this is the first Cold basic block update fgFirstColdBlock
                         if (bNext->IsFirstColdBlock(this))

src/coreclr/jit/flowgraph.cpp

@@ -306,19 +306,23 @@ BasicBlock* Compiler::fgCreateGCPoll(GCPollType pollType, BasicBlock* block)
 
         // Mark Poll as rarely run.
         poll->bbSetRunRarely();
-        poll->bbNatLoopNum = lpIndex; // Set the bbNatLoopNum in case we are in a loop
 
-        bottom->bbNatLoopNum = lpIndex; // Set the bbNatLoopNum in case we are in a loop
-        if (lpIndex != BasicBlock::NOT_IN_LOOP)
+        if (optLoopTableValid)
         {
-            // Set the new lpBottom in the natural loop table
-            optLoopTable[lpIndex].lpBottom = bottom;
-        }
+            poll->bbNatLoopNum = lpIndex; // Set the bbNatLoopNum in case we are in a loop
 
-        if (lpIndexFallThrough != BasicBlock::NOT_IN_LOOP)
-        {
-            // Set the new lpHead in the natural loop table
-            optLoopTable[lpIndexFallThrough].lpHead = bottom;
+            bottom->bbNatLoopNum = lpIndex; // Set the bbNatLoopNum in case we are in a loop
+            if (lpIndex != BasicBlock::NOT_IN_LOOP)
+            {
+                // Set the new lpBottom in the natural loop table
+                optLoopTable[lpIndex].lpBottom = bottom;
+            }
+
+            if (lpIndexFallThrough != BasicBlock::NOT_IN_LOOP)
+            {
+                // Set the new lpHead in the natural loop table
+                optLoopTable[lpIndexFallThrough].lpHead = bottom;
+            }
         }
 
         // Add the GC_CALL node to Poll.
@@ -2696,7 +2700,7 @@ bool Compiler::fgSimpleLowerCastOfSmpOp(LIR::Range& range, GenTreeCast* cast)
 //
 BasicBlock* Compiler::fgGetDomSpeculatively(const BasicBlock* block)
 {
-    assert(fgDomsComputed);
+    assert(fgSsaDomTree != nullptr);
     BasicBlock* lastReachablePred = nullptr;
 
     // Check if we have unreachable preds
@@ -2709,7 +2713,7 @@ BasicBlock* Compiler::fgGetDomSpeculatively(const BasicBlock* block)
         }
 
         // We check pred's count of InEdges - it's quite conservative.
-        // We, probably, could use fgReachable(fgFirstBb, pred) here to detect unreachable preds
+        // We, probably, could use optReachable(fgFirstBb, pred) here to detect unreachable preds
         if (predBlock->countOfInEdges() > 0)
         {
             if (lastReachablePred != nullptr)

src/coreclr/jit/morph.cpp

@@ -13193,8 +13193,8 @@ Compiler::FoldResult Compiler::fgFoldConditional(BasicBlock* block)
                 /* Unmark the loop if we are removing a backwards branch */
                 /* dest block must also be marked as a loop head and     */
                 /* We must be able to reach the backedge block           */
-                if (block->GetTrueTarget()->isLoopHead() && (block->GetTrueTarget()->bbNum <= block->bbNum) &&
-                    fgReachable(block->GetTrueTarget(), block))
+                if (optLoopTableValid && block->GetTrueTarget()->isLoopHead() &&
+                    (block->GetTrueTarget()->bbNum <= block->bbNum) && fgReachable(block->GetTrueTarget(), block))
                 {
                     optUnmarkLoopBlocks(block->GetTrueTarget(), block);
                 }

src/coreclr/jit/optimizebools.cpp

@@ -1339,10 +1339,13 @@ void OptBoolsDsc::optOptimizeBoolsUpdateTrees()
     }
 
     // Update loop table
-    m_comp->fgUpdateLoopsAfterCompacting(m_b1, m_b2);
-    if (optReturnBlock)
+    if (m_comp->optLoopTableValid)
     {
-        m_comp->fgUpdateLoopsAfterCompacting(m_b1, m_b3);
+        m_comp->fgUpdateLoopsAfterCompacting(m_b1, m_b2);
+        if (optReturnBlock)
+        {
+            m_comp->fgUpdateLoopsAfterCompacting(m_b1, m_b3);
+        }
     }
 
     // Update IL range of first block

src/coreclr/jit/optimizer.cpp

@@ -520,29 +520,6 @@ void Compiler::optUpdateLoopsBeforeRemoveBlock(BasicBlock* block, bool skipUnmar
     }
 }
 
-//------------------------------------------------------------------------
-// optClearLoopIterInfo: Clear the info related to LPFLG_ITER loops in the loop table.
-// The various fields related to iterators is known to be valid for loop cloning and unrolling,
-// but becomes invalid afterwards. Clear the info that might be used incorrectly afterwards
-// in JitDump or by subsequent phases.
-//
-PhaseStatus Compiler::optClearLoopIterInfo()
-{
-    for (unsigned lnum = 0; lnum < optLoopCount; lnum++)
-    {
-        LoopDsc& loop = optLoopTable[lnum];
-        loop.lpFlags &= ~(LPFLG_ITER | LPFLG_CONST_INIT | LPFLG_SIMD_LIMIT | LPFLG_VAR_LIMIT | LPFLG_CONST_LIMIT |
-                          LPFLG_ARRLEN_LIMIT);
-
-        loop.lpIterTree  = nullptr;
-        loop.lpInitBlock = nullptr;
-        loop.lpConstInit = -1;
-        loop.lpTestTree  = nullptr;
-    }
-
-    return PhaseStatus::MODIFIED_NOTHING;
-}
-
 #ifdef DEBUG
 
 /*****************************************************************************
@@ -5543,6 +5520,11 @@ void Compiler::optFindNewLoops()
     // edges, so be conservative here.
     fgMightHaveNaturalLoops = (m_loops->NumLoops() > 0) || m_loops->HaveNonNaturalLoopCycles();
 
+    for (BasicBlock* block : Blocks())
+    {
+        block->RemoveFlags(BBF_OLD_LOOP_HEADER_QUIRK);
+    }
+
     for (FlowGraphNaturalLoop* loop : m_loops->InReversePostOrder())
     {
         BasicBlock* head = loop->GetHeader();
@@ -8349,23 +8331,6 @@ bool Compiler::fgCreateLoopPreHeader(unsigned lnum)
     return true;
 }
 
-bool Compiler::optBlockIsLoopEntry(BasicBlock* blk, unsigned* pLnum)
-{
-    for (unsigned lnum = blk->bbNatLoopNum; lnum != BasicBlock::NOT_IN_LOOP; lnum = optLoopTable[lnum].lpParent)
-    {
-        if (optLoopTable[lnum].lpIsRemoved())
-        {
-            continue;
-        }
-        if (optLoopTable[lnum].lpEntry == blk)
-        {
-            *pLnum = lnum;
-            return true;
-        }
-    }
-    return false;
-}
-
 LoopSideEffects::LoopSideEffects() : VarInOut(VarSetOps::UninitVal()), VarUseDef(VarSetOps::UninitVal())
 {
     for (MemoryKind mk : allMemoryKinds())