From c57d1a5953f13a63f75fc1d2f47c35d195545a0e Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Mon, 11 Dec 2023 14:24:31 +0100 Subject: [PATCH 1/6] JIT: Null out loop table when invalid Null out the loop table as part of setting optLoopTableValid = false. This reveals a few cases where we were using it even after invalidation. A few diffs expected from the fgCanCompactBlocks change because we now compact some loop headers. --- src/coreclr/jit/compiler.cpp | 1 + src/coreclr/jit/compiler.h | 4 ---- src/coreclr/jit/fgopt.cpp | 14 ++++++++++---- src/coreclr/jit/flowgraph.cpp | 24 ++++++++++++++---------- src/coreclr/jit/optimizebools.cpp | 9 ++++++--- src/coreclr/jit/optimizer.cpp | 17 ----------------- 6 files changed, 31 insertions(+), 38 deletions(-) diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index f3eee4ec87bcdd..c0a62d0b683a0e 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -5016,6 +5016,7 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl fgDomsComputed = false; optLoopTableValid = false; optLoopsRequirePreHeaders = false; + optLoopTable = nullptr; #ifdef DEBUG DoPhase(this, PHASE_STRESS_SPLIT_TREE, &Compiler::StressSplitTree); diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 024811667394dd..1c590e10d12e50 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -6790,10 +6790,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(); diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 4e2eb544cfc548..85296c6cf76188 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/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 (optLoopTableValid && optIsLoopEntry(block)) { return false; } @@ -2404,7 +2404,10 @@ void Compiler::fgCompactBlocks(BasicBlock* block, BasicBlock* bNext) } } - fgUpdateLoopsAfterCompacting(block, bNext); + if (optLoopTableValid) + { + fgUpdateLoopsAfterCompacting(block, bNext); + } #if DEBUG if (verbose && 0) @@ -6210,8 +6213,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 block was aligned, unmark it bNext->unmarkLoopAlign(this DEBUG_ARG("Optimized jump")); diff --git a/src/coreclr/jit/flowgraph.cpp b/src/coreclr/jit/flowgraph.cpp index d1a7eb55192847..0ef298ba9bea29 100644 --- a/src/coreclr/jit/flowgraph.cpp +++ b/src/coreclr/jit/flowgraph.cpp @@ -300,19 +300,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. diff --git a/src/coreclr/jit/optimizebools.cpp b/src/coreclr/jit/optimizebools.cpp index ca159980a138fc..19af98edfcab2e 100644 --- a/src/coreclr/jit/optimizebools.cpp +++ b/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 diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index 0af1677258ce1c..bc714f1f828f0a 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -8333,23 +8333,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()) From e4e3a571bf25112cbe8f3a526168b557446f7a91 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Mon, 11 Dec 2023 21:25:06 +0100 Subject: [PATCH 2/6] Move further --- src/coreclr/jit/block.cpp | 4 ++++ src/coreclr/jit/block.h | 3 ++- src/coreclr/jit/compiler.cpp | 14 +++++++------- src/coreclr/jit/fgopt.cpp | 2 +- src/coreclr/jit/loopcloning.cpp | 5 +++++ src/coreclr/jit/optimizer.cpp | 4 ++++ 6 files changed, 23 insertions(+), 9 deletions(-) diff --git a/src/coreclr/jit/block.cpp b/src/coreclr/jit/block.cpp index e6e2abeb059c21..becd86c2003294 100644 --- a/src/coreclr/jit/block.cpp +++ b/src/coreclr/jit/block.cpp @@ -591,6 +591,10 @@ void BasicBlock::dspFlags() { printf("gcpoll "); } + if (HasFlag(BBF_OLD_LOOP_HEADER_QUIRK)) + { + printf("loopheader "); + } } /***************************************************************************** diff --git a/src/coreclr/jit/block.h b/src/coreclr/jit/block.h index 93fdc0358ce833..2fcba0de82933a 100644 --- a/src/coreclr/jit/block.h +++ b/src/coreclr/jit/block.h @@ -430,6 +430,7 @@ enum BasicBlockFlags : unsigned __int64 // and should be treated as if it falls through. // This is just to reduce diffs from removing BBJ_NONE. // (TODO: Remove this quirk after refactoring Compiler::fgFindInsertPoint) + BBF_CLONED_LOOP_HEADER = MAKE_BBFLAG(43), // The following are sets of flags. @@ -440,7 +441,7 @@ enum BasicBlockFlags : unsigned __int64 // Flags to update when two blocks are compacted BBF_COMPACT_UPD = BBF_GC_SAFE_POINT | BBF_NEEDS_GCPOLL | BBF_HAS_JMP | BBF_HAS_IDX_LEN | BBF_HAS_MD_IDX_LEN | BBF_BACKWARD_JUMP | \ - BBF_HAS_NEWOBJ | BBF_HAS_NULLCHECK | BBF_HAS_MDARRAYREF | BBF_LOOP_PREHEADER, + BBF_HAS_NEWOBJ | BBF_HAS_NULLCHECK | BBF_HAS_MDARRAYREF | BBF_LOOP_PREHEADER | BBF_OLD_LOOP_HEADER_QUIRK, // Flags a block should not have had before it is split. diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index c0a62d0b683a0e..633b9fa9ecaadb 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -4988,6 +4988,13 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl DoPhase(this, PHASE_VN_BASED_DEAD_STORE_REMOVAL, &Compiler::optVNBasedDeadStoreRemoval); } + // Dominator and reachability sets are no longer valid. + // The loop table is no longer valid. + fgDomsComputed = false; + optLoopTableValid = false; + optLoopsRequirePreHeaders = false; + optLoopTable = nullptr; + if (fgModified) { // update the flowgraph if we modified it during the optimization phase @@ -5011,13 +5018,6 @@ 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; - optLoopTable = nullptr; - #ifdef DEBUG DoPhase(this, PHASE_STRESS_SPLIT_TREE, &Compiler::StressSplitTree); #endif diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 85296c6cf76188..c31789d631bd28 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/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 (optLoopTableValid && optIsLoopEntry(block)) + if (block->HasFlag(BBF_OLD_LOOP_HEADER_QUIRK)) { return false; } diff --git a/src/coreclr/jit/loopcloning.cpp b/src/coreclr/jit/loopcloning.cpp index b6e52bd9a09e84..57e10c6638767f 100644 --- a/src/coreclr/jit/loopcloning.cpp +++ b/src/coreclr/jit/loopcloning.cpp @@ -2208,6 +2208,11 @@ void Compiler::optCloneLoop(FlowGraphNaturalLoop* loop, LoopCloneContext* contex return BasicBlockVisit::Continue; }); + BasicBlock* newHeader = nullptr; + bool result = blockMap->Lookup(loop->GetHeader(), &newHeader); + assert(result); + newHeader->SetFlags(BBF_CLONED_LOOP_HEADER); + // Perform the static optimizations on the fast path. optPerformStaticOptimizations(loop, context DEBUGARG(true)); diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index bc714f1f828f0a..00f6b39197f57a 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -5553,6 +5553,9 @@ void Compiler::optFindNewLoops() m_newToOldLoop = m_loops->NumLoops() == 0 ? nullptr : (new (this, CMK_Loops) LoopDsc*[m_loops->NumLoops()]{}); m_oldToNewLoop = new (this, CMK_Loops) FlowGraphNaturalLoop*[BasicBlock::MAX_LOOP_NUM]{}; + for (BasicBlock* block : Blocks()) + block->RemoveFlags(BBF_OLD_LOOP_HEADER_QUIRK); + for (FlowGraphNaturalLoop* loop : m_loops->InReversePostOrder()) { BasicBlock* head = loop->GetHeader(); @@ -5567,6 +5570,7 @@ void Compiler::optFindNewLoops() assert(m_newToOldLoop[loop->GetIndex()] == nullptr); m_oldToNewLoop[head->bbNatLoopNum] = loop; m_newToOldLoop[loop->GetIndex()] = dsc; + head->SetFlags(BBF_OLD_LOOP_HEADER_QUIRK); } } From 4e3bae48d4bda6575ec3416a1389c8aa4051d449 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Mon, 11 Dec 2023 22:06:12 +0100 Subject: [PATCH 3/6] Further quirking --- src/coreclr/jit/block.h | 2 +- src/coreclr/jit/compiler.cpp | 11 +++++++---- src/coreclr/jit/compiler.h | 1 + src/coreclr/jit/fgbasic.cpp | 1 + src/coreclr/jit/fgopt.cpp | 30 +++++++++++++++++++----------- src/coreclr/jit/loopcloning.cpp | 5 ----- 6 files changed, 29 insertions(+), 21 deletions(-) diff --git a/src/coreclr/jit/block.h b/src/coreclr/jit/block.h index 2fcba0de82933a..7cf1d6283c739d 100644 --- a/src/coreclr/jit/block.h +++ b/src/coreclr/jit/block.h @@ -430,7 +430,7 @@ enum BasicBlockFlags : unsigned __int64 // and should be treated as if it falls through. // This is just to reduce diffs from removing BBJ_NONE. // (TODO: Remove this quirk after refactoring Compiler::fgFindInsertPoint) - BBF_CLONED_LOOP_HEADER = MAKE_BBFLAG(43), + BBF_OLD_LOOP_HEADER_QUIRK = MAKE_BBFLAG(42), // The following are sets of flags. diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index 633b9fa9ecaadb..1af50e649667f7 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -4990,10 +4990,10 @@ 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; - optLoopTable = nullptr; + fgCompactRenumberQuirk = true; + fgDomsComputed = false; + optLoopTableValid = false; + optLoopTable = nullptr; if (fgModified) { @@ -5018,6 +5018,9 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl } } + optLoopsRequirePreHeaders = false; + fgCompactRenumberQuirk = false; + #ifdef DEBUG DoPhase(this, PHASE_STRESS_SPLIT_TREE, &Compiler::StressSplitTree); #endif diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 1c590e10d12e50..6625c042e562b2 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -5097,6 +5097,7 @@ 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 fgCompactRenumberQuirk; // Should fgCompactBlocks renumber BBs above fgDomBBcount? TODO-Quirk: Remove bool fgHasSwitch; // any BBJ_SWITCH jumps? diff --git a/src/coreclr/jit/fgbasic.cpp b/src/coreclr/jit/fgbasic.cpp index d701a9b4b4bec5..b91fd104a9412c 100644 --- a/src/coreclr/jit/fgbasic.cpp +++ b/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; diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index c31789d631bd28..be8f7f67298b5a 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -2372,20 +2372,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; diff --git a/src/coreclr/jit/loopcloning.cpp b/src/coreclr/jit/loopcloning.cpp index 57e10c6638767f..b6e52bd9a09e84 100644 --- a/src/coreclr/jit/loopcloning.cpp +++ b/src/coreclr/jit/loopcloning.cpp @@ -2208,11 +2208,6 @@ void Compiler::optCloneLoop(FlowGraphNaturalLoop* loop, LoopCloneContext* contex return BasicBlockVisit::Continue; }); - BasicBlock* newHeader = nullptr; - bool result = blockMap->Lookup(loop->GetHeader(), &newHeader); - assert(result); - newHeader->SetFlags(BBF_CLONED_LOOP_HEADER); - // Perform the static optimizations on the fast path. optPerformStaticOptimizations(loop, context DEBUGARG(true)); From 616f01a7b7a0487339959c827c913eea68625f5d Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Mon, 11 Dec 2023 22:47:29 +0100 Subject: [PATCH 4/6] Move all the way back --- src/coreclr/jit/compiler.cpp | 17 ++++---- src/coreclr/jit/compphases.h | 1 - src/coreclr/jit/fgdiagnostic.cpp | 1 - src/coreclr/jit/flowgraph.cpp | 4 +- src/coreclr/jit/morph.cpp | 4 +- src/coreclr/jit/redundantbranchopts.cpp | 55 +++++++++++++------------ 6 files changed, 40 insertions(+), 42 deletions(-) diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index 1af50e649667f7..5543d0e968ee91 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -4828,9 +4828,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 @@ -4847,6 +4847,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()); @@ -4988,13 +4992,6 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl DoPhase(this, PHASE_VN_BASED_DEAD_STORE_REMOVAL, &Compiler::optVNBasedDeadStoreRemoval); } - // Dominator and reachability sets are no longer valid. - // The loop table is no longer valid. - fgCompactRenumberQuirk = true; - fgDomsComputed = false; - optLoopTableValid = false; - optLoopTable = nullptr; - if (fgModified) { // update the flowgraph if we modified it during the optimization phase diff --git a/src/coreclr/jit/compphases.h b/src/coreclr/jit/compphases.h index 8e6b7123ea5b7b..d181ecc1bb9548 100644 --- a/src/coreclr/jit/compphases.h +++ b/src/coreclr/jit/compphases.h @@ -69,7 +69,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) diff --git a/src/coreclr/jit/fgdiagnostic.cpp b/src/coreclr/jit/fgdiagnostic.cpp index 77b0c89364136c..7338a30f7c9c6e 100644 --- a/src/coreclr/jit/fgdiagnostic.cpp +++ b/src/coreclr/jit/fgdiagnostic.cpp @@ -4753,7 +4753,6 @@ void Compiler::fgDebugCheckSsa() } assert(fgSsaPassesCompleted > 0); - assert(fgDomsComputed); // Visit the blocks that SSA initially renamed // diff --git a/src/coreclr/jit/flowgraph.cpp b/src/coreclr/jit/flowgraph.cpp index 0ef298ba9bea29..b12e0c8a3728d5 100644 --- a/src/coreclr/jit/flowgraph.cpp +++ b/src/coreclr/jit/flowgraph.cpp @@ -2719,7 +2719,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 @@ -2732,7 +2732,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) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index efe17974d392cd..495917db63ff15 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -13192,8 +13192,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->GetJumpDest()->isLoopHead() && (block->GetJumpDest()->bbNum <= block->bbNum) && - fgReachable(block->GetJumpDest(), block)) + if (optLoopTableValid && block->GetJumpDest()->isLoopHead() && + (block->GetJumpDest()->bbNum <= block->bbNum) && fgReachable(block->GetJumpDest(), block)) { optUnmarkLoopBlocks(block->GetJumpDest(), block); } diff --git a/src/coreclr/jit/redundantbranchopts.cpp b/src/coreclr/jit/redundantbranchopts.cpp index b4a418c9b0b491..b70398f896593e 100644 --- a/src/coreclr/jit/redundantbranchopts.cpp +++ b/src/coreclr/jit/redundantbranchopts.cpp @@ -779,36 +779,39 @@ bool Compiler::optJumpThreadCheck(BasicBlock* const block, BasicBlock* const dom return false; } - // If block is a loop header, skip jump threading. - // - // This is an artificial limitation to ensure that subsequent loop table valididity - // checking can pass. We do not expect a loop entry to have multiple non-loop predecessors. - // - // This only blocks jump threading in a small number of cases. - // Revisit once we can ensure that loop headers are not critical blocks. - // - // Likewise we can't properly update the loop table if we thread the entry block. - // Not clear how much impact this has. - // - for (unsigned loopNum = 0; loopNum < optLoopCount; loopNum++) + if (optLoopTableValid) { - const LoopDsc& loop = optLoopTable[loopNum]; - - if (loop.lpIsRemoved()) + // If block is a loop header, skip jump threading. + // + // This is an artificial limitation to ensure that subsequent loop table valididity + // checking can pass. We do not expect a loop entry to have multiple non-loop predecessors. + // + // This only blocks jump threading in a small number of cases. + // Revisit once we can ensure that loop headers are not critical blocks. + // + // Likewise we can't properly update the loop table if we thread the entry block. + // Not clear how much impact this has. + // + for (unsigned loopNum = 0; loopNum < optLoopCount; loopNum++) { - continue; - } + const LoopDsc& loop = optLoopTable[loopNum]; - if (block == loop.lpHead) - { - JITDUMP(FMT_BB " is the header for " FMT_LP "; no threading\n", block->bbNum, loopNum); - return false; - } + if (loop.lpIsRemoved()) + { + continue; + } - if (block == loop.lpEntry) - { - JITDUMP(FMT_BB " is the entry for " FMT_LP "; no threading\n", block->bbNum, loopNum); - return false; + if (block == loop.lpHead) + { + JITDUMP(FMT_BB " is the header for " FMT_LP "; no threading\n", block->bbNum, loopNum); + return false; + } + + if (block == loop.lpEntry) + { + JITDUMP(FMT_BB " is the entry for " FMT_LP "; no threading\n", block->bbNum, loopNum); + return false; + } } } From ac329ddd754876a1db48c0cca04dd6055613c767 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Mon, 11 Dec 2023 22:55:14 +0100 Subject: [PATCH 5/6] Further cleanup --- src/coreclr/jit/compiler.h | 2 -- src/coreclr/jit/morph.cpp | 4 ++-- src/coreclr/jit/optimizer.cpp | 23 ----------------------- 3 files changed, 2 insertions(+), 27 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 34ad480ce96584..e8f5fcc6438b5b 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -7097,8 +7097,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); diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 3779ef5f6a096c..77e87872328ded 100644 --- a/src/coreclr/jit/morph.cpp +++ b/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 (optLoopTableValid && 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); } diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index 0232572d298311..3400426e0ad272 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -522,29 +522,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 /***************************************************************************** From 630bfaaf30677cefde105b584e4075e7aeef3b60 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 12 Dec 2023 08:56:46 +0100 Subject: [PATCH 6/6] Fix JitOptRepeat --- src/coreclr/jit/compiler.cpp | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index 5f09a9034e3c49..12c6c1c0d5503a 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -5898,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; } /*****************************************************************************/