From 83c6fbd7bc3721b7daeff44404960044d96d047e Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Fri, 8 Mar 2024 16:46:35 -0500 Subject: [PATCH 1/5] Template PredBlocks() --- src/coreclr/jit/block.h | 47 ++++++++++++++++++++++++++++------------- 1 file changed, 32 insertions(+), 15 deletions(-) diff --git a/src/coreclr/jit/block.h b/src/coreclr/jit/block.h index 9627cb6395c965..02bbfcad4181ab 100644 --- a/src/coreclr/jit/block.h +++ b/src/coreclr/jit/block.h @@ -258,7 +258,9 @@ class PredEdgeList // PredBlockList: adapter class for forward iteration of the predecessor edge linked list yielding // predecessor blocks, using range-based `for`, normally used via BasicBlock::PredBlocks(), e.g.: // for (BasicBlock* const predBlock : block->PredBlocks()) ... +// allowEdits controls whether the iterator should be resilient to changes to the predecessor list. // +template class PredBlockList { FlowEdge* m_begin; @@ -270,13 +272,12 @@ class PredBlockList { FlowEdge* m_pred; -#ifdef DEBUG - // Try to guard against the user of the iterator from making changes to the IR that would invalidate - // the iterator: cache the edge we think should be next, then check it when we actually do the `++` + // When allowEdits=false, try to guard against the user of the iterator from modifying the predecessor list + // being traversed: cache the edge we think should be next, then check it when we actually do the `++` // operation. This is a bit conservative, but attempts to protect against callers assuming too much about // this iterator implementation. + // When allowEdits=true, m_next is always used to update m_pred, so changes to m_pred don't break the iterator. FlowEdge* m_next; -#endif public: iterator(FlowEdge* pred); @@ -1530,9 +1531,10 @@ struct BasicBlock : private LIR::Range // PredBlocks: convenience method for enabling range-based `for` iteration over predecessor blocks, e.g.: // for (BasicBlock* const predBlock : block->PredBlocks()) ... // - PredBlockList PredBlocks() const + template + PredBlockList PredBlocks() const { - return PredBlockList(bbPreds); + return PredBlockList(bbPreds); } // Pred list maintenance @@ -2399,28 +2401,43 @@ inline PredEdgeList::iterator& PredEdgeList::iterator::operator++() return *this; } -inline PredBlockList::iterator::iterator(FlowEdge* pred) : m_pred(pred) +template +inline PredBlockList::iterator::iterator(FlowEdge* pred) : m_pred(pred) { -#ifdef DEBUG - m_next = (m_pred == nullptr) ? nullptr : m_pred->getNextPredEdge(); -#endif + bool initNextPointer = allowEdits; + INDEBUG(initNextPointer = true); + if (initNextPointer) + { + m_next = (m_pred == nullptr) ? nullptr : m_pred->getNextPredEdge(); + } } -inline BasicBlock* PredBlockList::iterator::operator*() const +template +inline BasicBlock* PredBlockList::iterator::operator*() const { return m_pred->getSourceBlock(); } -inline PredBlockList::iterator& PredBlockList::iterator::operator++() +template +inline typename PredBlockList::iterator& PredBlockList::iterator::operator++() { FlowEdge* next = m_pred->getNextPredEdge(); + bool updateNextPointer = allowEdits; #ifdef DEBUG - // Check that the next block is the one we expect to see. - assert(next == m_next); - m_next = (next == nullptr) ? nullptr : next->getNextPredEdge(); + // If allowEdits=false, check that the next block is the one we expect to see. + // If allowEdits=true, the user can modify the predecessor list while traversing it, + // so (next == m_next) may not be true. Since we cached the next pointer in m_next, this won't break iteration. + assert((next == m_next) || allowEdits); + updateNextPointer = true; #endif // DEBUG + if (updateNextPointer) + { + next = m_next; + m_next = (m_next == nullptr) ? nullptr : m_next->getNextPredEdge(); + } + m_pred = next; return *this; } From bc018d1157a4cd7eb6eb9395b1ca1382d854e51e Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Fri, 8 Mar 2024 17:15:48 -0500 Subject: [PATCH 2/5] Update fgReplaceJumpTarget --- src/coreclr/jit/block.h | 12 +++---- src/coreclr/jit/fgbasic.cpp | 11 ++---- src/coreclr/jit/fgehopt.cpp | 48 +++++++++---------------- src/coreclr/jit/optimizer.cpp | 6 ++-- src/coreclr/jit/redundantbranchopts.cpp | 3 +- 5 files changed, 32 insertions(+), 48 deletions(-) diff --git a/src/coreclr/jit/block.h b/src/coreclr/jit/block.h index 02bbfcad4181ab..fbf707d3eaf268 100644 --- a/src/coreclr/jit/block.h +++ b/src/coreclr/jit/block.h @@ -1531,7 +1531,7 @@ struct BasicBlock : private LIR::Range // PredBlocks: convenience method for enabling range-based `for` iteration over predecessor blocks, e.g.: // for (BasicBlock* const predBlock : block->PredBlocks()) ... // - template + template PredBlockList PredBlocks() const { return PredBlockList(bbPreds); @@ -2412,17 +2412,17 @@ inline PredBlockList::iterator::iterator(FlowEdge* pred) : m_pred(pr } } -template +template inline BasicBlock* PredBlockList::iterator::operator*() const { return m_pred->getSourceBlock(); } -template +template inline typename PredBlockList::iterator& PredBlockList::iterator::operator++() { - FlowEdge* next = m_pred->getNextPredEdge(); - bool updateNextPointer = allowEdits; + FlowEdge* next = m_pred->getNextPredEdge(); + bool updateNextPointer = allowEdits; #ifdef DEBUG // If allowEdits=false, check that the next block is the one we expect to see. @@ -2434,7 +2434,7 @@ inline typename PredBlockList::iterator& PredBlockList:: if (updateNextPointer) { - next = m_next; + next = m_next; m_next = (m_next == nullptr) ? nullptr : m_next->getNextPredEdge(); } diff --git a/src/coreclr/jit/fgbasic.cpp b/src/coreclr/jit/fgbasic.cpp index 0eb618f1777222..3cb7fd03863f29 100644 --- a/src/coreclr/jit/fgbasic.cpp +++ b/src/coreclr/jit/fgbasic.cpp @@ -645,17 +645,11 @@ void Compiler::fgReplaceJumpTarget(BasicBlock* block, BasicBlock* oldTarget, Bas case BBJ_EHCATCHRET: case BBJ_EHFILTERRET: case BBJ_LEAVE: // This function can be called before import, so we still have BBJ_LEAVE - { - // TODO: Use fgRedirectTargetEdge once pred edge iterators are resilient to bbPreds being modified. assert(block->TargetIs(oldTarget)); - fgRemoveRefPred(block->GetTargetEdge()); - FlowEdge* const newEdge = fgAddRefPred(newTarget, block, block->GetTargetEdge()); - block->SetTargetEdge(newEdge); + fgRedirectTargetEdge(block, newTarget); break; - } case BBJ_COND: - if (block->TrueTargetIs(oldTarget)) { FlowEdge* const oldEdge = block->GetTrueEdge(); @@ -5297,7 +5291,8 @@ BasicBlock* Compiler::fgRemoveBlock(BasicBlock* block, bool unreachable) fgRemoveRefPred(block->GetTargetEdge()); - for (BasicBlock* const predBlock : block->PredBlocks()) + constexpr bool allowEdits = true; + for (BasicBlock* const predBlock : block->PredBlocks()) { /* change all jumps/refs to the removed block */ switch (predBlock->GetKind()) diff --git a/src/coreclr/jit/fgehopt.cpp b/src/coreclr/jit/fgehopt.cpp index 743841fec24291..8f1f76f771d997 100644 --- a/src/coreclr/jit/fgehopt.cpp +++ b/src/coreclr/jit/fgehopt.cpp @@ -2002,36 +2002,31 @@ PhaseStatus Compiler::fgTailMergeThrows() // Walk pred list of the non canonical block, updating flow to target // the canonical block instead. - for (FlowEdge* predEdge = nonCanonicalBlock->bbPreds; predEdge != nullptr; predEdge = nextPredEdge) + constexpr bool allowEdits = true; + for (BasicBlock* const predBlock : nonCanonicalBlock->PredBlocks()) { - BasicBlock* const predBlock = predEdge->getSourceBlock(); - nextPredEdge = predEdge->getNextPredEdge(); - switch (predBlock->GetKind()) { - case BBJ_ALWAYS: - { - fgTailMergeThrowsJumpToHelper(predBlock, nonCanonicalBlock, canonicalBlock, predEdge); - updated = true; - } - break; - + // TODO: Use fgReplaceJumpTarget once it preserves edge weights for BBJ_COND blocks case BBJ_COND: { - // Flow to non canonical block could be via fall through or jump or both. + // Flow to non canonical block could be via true or false target. if (predBlock->FalseTargetIs(nonCanonicalBlock)) { - fgTailMergeThrowsFallThroughHelper(predBlock, nonCanonicalBlock, canonicalBlock, predEdge); + fgTailMergeThrowsFallThroughHelper(predBlock, nonCanonicalBlock, canonicalBlock, + predBlock->GetFalseEdge()); } if (predBlock->TrueTargetIs(nonCanonicalBlock)) { - fgTailMergeThrowsJumpToHelper(predBlock, nonCanonicalBlock, canonicalBlock, predEdge); + fgTailMergeThrowsJumpToHelper(predBlock, nonCanonicalBlock, canonicalBlock, + predBlock->GetTrueEdge()); } updated = true; } break; + case BBJ_ALWAYS: case BBJ_SWITCH: { JITDUMP("*** " FMT_BB " now branching to " FMT_BB "\n", predBlock->bbNum, canonicalBlock->bbNum); @@ -2127,21 +2122,12 @@ void Compiler::fgTailMergeThrowsJumpToHelper(BasicBlock* predBlock, { JITDUMP("*** " FMT_BB " now branching to " FMT_BB "\n", predBlock->bbNum, canonicalBlock->bbNum); - if (predBlock->KindIs(BBJ_ALWAYS)) - { - // Update flow to new target - assert(predBlock->TargetIs(nonCanonicalBlock)); - fgRedirectTargetEdge(predBlock, canonicalBlock); - } - else - { - // Remove the old flow - assert(predBlock->KindIs(BBJ_COND)); - assert(predBlock->TrueTargetIs(nonCanonicalBlock)); - fgRemoveRefPred(predBlock->GetTrueEdge()); - - // Wire up the new flow - FlowEdge* const trueEdge = fgAddRefPred(canonicalBlock, predBlock, predEdge); - predBlock->SetTrueEdge(trueEdge); - } + // Remove the old flow + assert(predBlock->KindIs(BBJ_COND)); + assert(predBlock->TrueTargetIs(nonCanonicalBlock)); + fgRemoveRefPred(predBlock->GetTrueEdge()); + + // Wire up the new flow + FlowEdge* const trueEdge = fgAddRefPred(canonicalBlock, predBlock, predEdge); + predBlock->SetTrueEdge(trueEdge); } diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index 212a4420e11084..fdb70e455034bf 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -2247,7 +2247,8 @@ bool Compiler::optInvertWhileLoop(BasicBlock* block) // unsigned const loopFirstNum = bTop->bbNum; unsigned const loopBottomNum = bTest->bbNum; - for (BasicBlock* const predBlock : bTest->PredBlocks()) + constexpr bool allowEdits = true; + for (BasicBlock* const predBlock : bTest->PredBlocks()) { unsigned const bNum = predBlock->bbNum; if ((loopFirstNum <= bNum) && (bNum <= loopBottomNum)) @@ -3154,7 +3155,8 @@ bool Compiler::optCanonicalizeExit(FlowGraphNaturalLoop* loop, BasicBlock* exit) JITDUMP("Created new exit " FMT_BB " to replace " FMT_BB " for " FMT_LP "\n", newExit->bbNum, exit->bbNum, loop->GetIndex()); - for (BasicBlock* pred : exit->PredBlocks()) + constexpr bool allowEdits = true; + for (BasicBlock* pred : exit->PredBlocks()) { if (loop->ContainsBlock(pred)) { diff --git a/src/coreclr/jit/redundantbranchopts.cpp b/src/coreclr/jit/redundantbranchopts.cpp index 9dd9a2959e76a3..4175863d5b5847 100644 --- a/src/coreclr/jit/redundantbranchopts.cpp +++ b/src/coreclr/jit/redundantbranchopts.cpp @@ -1600,7 +1600,8 @@ bool Compiler::optJumpThreadCore(JumpThreadInfo& jti) // If this pred is in the set that will reuse block, do nothing. // Else revise pred to branch directly to the appropriate successor of block. // - for (BasicBlock* const predBlock : jti.m_block->PredBlocks()) + constexpr bool allowEdits = true; + for (BasicBlock* const predBlock : jti.m_block->PredBlocks()) { // If this was an ambiguous pred, skip. // From cd18ab07c805fc8239447cc5bc3667e77fdf1269 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Sun, 10 Mar 2024 18:48:24 -0400 Subject: [PATCH 3/5] Rename to PredBlocksEditing --- src/coreclr/jit/block.h | 14 +++++++++++--- src/coreclr/jit/fgbasic.cpp | 3 +-- src/coreclr/jit/fgehopt.cpp | 3 +-- src/coreclr/jit/optimizer.cpp | 6 ++---- src/coreclr/jit/redundantbranchopts.cpp | 3 +-- 5 files changed, 16 insertions(+), 13 deletions(-) diff --git a/src/coreclr/jit/block.h b/src/coreclr/jit/block.h index fbf707d3eaf268..9a9f01fa93a5db 100644 --- a/src/coreclr/jit/block.h +++ b/src/coreclr/jit/block.h @@ -1531,10 +1531,18 @@ struct BasicBlock : private LIR::Range // PredBlocks: convenience method for enabling range-based `for` iteration over predecessor blocks, e.g.: // for (BasicBlock* const predBlock : block->PredBlocks()) ... // - template - PredBlockList PredBlocks() const + PredBlockList PredBlocks() const { - return PredBlockList(bbPreds); + return PredBlockList(bbPreds); + } + + // PredBlocksEditing: convenience method for enabling range-based `for` iteration over predecessor blocks, e.g.: + // for (BasicBlock* const predBlock : block->PredBlocksList()) ... + // This iterator tolerates modifications to bbPreds. + // + PredBlockList PredBlocksEditing() const + { + return PredBlockList(bbPreds); } // Pred list maintenance diff --git a/src/coreclr/jit/fgbasic.cpp b/src/coreclr/jit/fgbasic.cpp index 3cb7fd03863f29..98c57006dae295 100644 --- a/src/coreclr/jit/fgbasic.cpp +++ b/src/coreclr/jit/fgbasic.cpp @@ -5291,8 +5291,7 @@ BasicBlock* Compiler::fgRemoveBlock(BasicBlock* block, bool unreachable) fgRemoveRefPred(block->GetTargetEdge()); - constexpr bool allowEdits = true; - for (BasicBlock* const predBlock : block->PredBlocks()) + for (BasicBlock* const predBlock : block->PredBlocksEditing()) { /* change all jumps/refs to the removed block */ switch (predBlock->GetKind()) diff --git a/src/coreclr/jit/fgehopt.cpp b/src/coreclr/jit/fgehopt.cpp index 8f1f76f771d997..a284b318d2e57d 100644 --- a/src/coreclr/jit/fgehopt.cpp +++ b/src/coreclr/jit/fgehopt.cpp @@ -2002,8 +2002,7 @@ PhaseStatus Compiler::fgTailMergeThrows() // Walk pred list of the non canonical block, updating flow to target // the canonical block instead. - constexpr bool allowEdits = true; - for (BasicBlock* const predBlock : nonCanonicalBlock->PredBlocks()) + for (BasicBlock* const predBlock : nonCanonicalBlock->PredBlocksEditing()) { switch (predBlock->GetKind()) { diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index fdb70e455034bf..41c9f73edaf4f4 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -2247,8 +2247,7 @@ bool Compiler::optInvertWhileLoop(BasicBlock* block) // unsigned const loopFirstNum = bTop->bbNum; unsigned const loopBottomNum = bTest->bbNum; - constexpr bool allowEdits = true; - for (BasicBlock* const predBlock : bTest->PredBlocks()) + for (BasicBlock* const predBlock : bTest->PredBlocksEditing()) { unsigned const bNum = predBlock->bbNum; if ((loopFirstNum <= bNum) && (bNum <= loopBottomNum)) @@ -3155,8 +3154,7 @@ bool Compiler::optCanonicalizeExit(FlowGraphNaturalLoop* loop, BasicBlock* exit) JITDUMP("Created new exit " FMT_BB " to replace " FMT_BB " for " FMT_LP "\n", newExit->bbNum, exit->bbNum, loop->GetIndex()); - constexpr bool allowEdits = true; - for (BasicBlock* pred : exit->PredBlocks()) + for (BasicBlock* pred : exit->PredBlocksEditing()) { if (loop->ContainsBlock(pred)) { diff --git a/src/coreclr/jit/redundantbranchopts.cpp b/src/coreclr/jit/redundantbranchopts.cpp index 4175863d5b5847..e8b346faccc376 100644 --- a/src/coreclr/jit/redundantbranchopts.cpp +++ b/src/coreclr/jit/redundantbranchopts.cpp @@ -1600,8 +1600,7 @@ bool Compiler::optJumpThreadCore(JumpThreadInfo& jti) // If this pred is in the set that will reuse block, do nothing. // Else revise pred to branch directly to the appropriate successor of block. // - constexpr bool allowEdits = true; - for (BasicBlock* const predBlock : jti.m_block->PredBlocks()) + for (BasicBlock* const predBlock : jti.m_block->PredBlocksEditing()) { // If this was an ambiguous pred, skip. // From 12f1580803f8716f19c503a4f502cd666e8681e3 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Sun, 10 Mar 2024 19:05:49 -0400 Subject: [PATCH 4/5] Slight refactor --- src/coreclr/jit/block.h | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/coreclr/jit/block.h b/src/coreclr/jit/block.h index 9a9f01fa93a5db..3b51ef8f35dfe2 100644 --- a/src/coreclr/jit/block.h +++ b/src/coreclr/jit/block.h @@ -2429,24 +2429,24 @@ inline BasicBlock* PredBlockList::iterator::operator*() const template inline typename PredBlockList::iterator& PredBlockList::iterator::operator++() { - FlowEdge* next = m_pred->getNextPredEdge(); - bool updateNextPointer = allowEdits; + if (allowEdits) + { + m_pred = m_next; + m_next = (m_next == nullptr) ? nullptr : m_next->getNextPredEdge(); + } + else + { + FlowEdge* next = m_pred->getNextPredEdge(); #ifdef DEBUG - // If allowEdits=false, check that the next block is the one we expect to see. - // If allowEdits=true, the user can modify the predecessor list while traversing it, - // so (next == m_next) may not be true. Since we cached the next pointer in m_next, this won't break iteration. - assert((next == m_next) || allowEdits); - updateNextPointer = true; + // If allowEdits=false, check that the next block is the one we expect to see. + assert(next == m_next); + m_next = (m_next == nullptr) ? nullptr : m_next->getNextPredEdge(); #endif // DEBUG - if (updateNextPointer) - { - next = m_next; - m_next = (m_next == nullptr) ? nullptr : m_next->getNextPredEdge(); + m_pred = next; } - m_pred = next; return *this; } From 032628901f926c6ae16cb0acae3f8c8f9f3a8db5 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Sun, 10 Mar 2024 19:07:57 -0400 Subject: [PATCH 5/5] Add comment --- src/coreclr/jit/block.h | 1 + 1 file changed, 1 insertion(+) diff --git a/src/coreclr/jit/block.h b/src/coreclr/jit/block.h index 3b51ef8f35dfe2..cd2a79fd098a5b 100644 --- a/src/coreclr/jit/block.h +++ b/src/coreclr/jit/block.h @@ -2431,6 +2431,7 @@ inline typename PredBlockList::iterator& PredBlockList:: { if (allowEdits) { + // For editing iterators, m_next is always used and maintained m_pred = m_next; m_next = (m_next == nullptr) ? nullptr : m_next->getNextPredEdge(); }