From 189b526f35794e4b90dff3465573ed7d26925663 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Mon, 30 Dec 2024 23:29:22 -0500 Subject: [PATCH 1/2] Move shared pred edge iterator logic to base class --- src/coreclr/jit/block.h | 157 +++++++++++++++++++--------------------- 1 file changed, 73 insertions(+), 84 deletions(-) diff --git a/src/coreclr/jit/block.h b/src/coreclr/jit/block.h index 03770815f0125a..5e41875e8a4be0 100644 --- a/src/coreclr/jit/block.h +++ b/src/coreclr/jit/block.h @@ -204,42 +204,60 @@ struct allMemoryKinds } }; +// Base class for forward iterators over the predecessor edge linked list. +// Subclasses decide what the iterator should yield (edge, source block, etc.). +// The pred list cannot be modified during iteration unless allowEdits is true. +// +template +class BasePredIterator +{ +private: + // 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; + +protected: + FlowEdge* m_pred; + +public: + BasePredIterator(FlowEdge* pred); + + virtual IteratorType* operator*() const = 0; + + BasePredIterator& operator++(); + + bool operator!=(const BasePredIterator& i) const + { + return m_pred != i.m_pred; + } +}; + // PredEdgeList: adapter class for forward iteration of the predecessor edge linked list using range-based `for`, // normally used via BasicBlock::PredEdges(), e.g.: // for (FlowEdge* const edge : block->PredEdges()) ... +// allowEdits controls whether the iterator should be resilient to changes to the predecessor list. // +template class PredEdgeList { FlowEdge* m_begin; // Forward iterator for the predecessor edges linked list. - // The caller can't make changes to the preds list when using this. // - class iterator + class PredEdgeIterator : public BasePredIterator { - 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 `++` - // operation. This is a bit conservative, but attempts to protect against callers assuming too much about - // this iterator implementation. - FlowEdge* m_next; -#endif - public: - iterator(FlowEdge* pred); - - FlowEdge* operator*() const + PredEdgeIterator(FlowEdge* pred) + : BasePredIterator(pred) { - return m_pred; } - iterator& operator++(); - - bool operator!=(const iterator& i) const + FlowEdge* operator*() const override { - return m_pred != i.m_pred; + return this->m_pred; } }; @@ -249,14 +267,14 @@ class PredEdgeList { } - iterator begin() const + PredEdgeIterator begin() const { - return iterator(m_begin); + return PredEdgeIterator(m_begin); } - iterator end() const + PredEdgeIterator end() const { - return iterator(nullptr); + return PredEdgeIterator(nullptr); } }; @@ -271,30 +289,16 @@ class PredBlockList FlowEdge* m_begin; // Forward iterator for the predecessor edges linked list, yielding the predecessor block, not the edge. - // The caller can't make changes to the preds list when using this. // - class iterator + class PredBlockIterator : public BasePredIterator { - FlowEdge* m_pred; - - // 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; - public: - iterator(FlowEdge* pred); - - BasicBlock* operator*() const; - - iterator& operator++(); - - bool operator!=(const iterator& i) const + PredBlockIterator(FlowEdge* pred) + : BasePredIterator(pred) { - return m_pred != i.m_pred; } + + BasicBlock* operator*() const override; }; public: @@ -303,14 +307,14 @@ class PredBlockList { } - iterator begin() const + PredBlockIterator begin() const { - return iterator(m_begin); + return PredBlockIterator(m_begin); } - iterator end() const + PredBlockIterator end() const { - return iterator(nullptr); + return PredBlockIterator(nullptr); } }; @@ -1536,9 +1540,18 @@ struct BasicBlock : private LIR::Range // PredEdges: convenience method for enabling range-based `for` iteration over predecessor edges, e.g.: // for (FlowEdge* const edge : block->PredEdges()) ... // - PredEdgeList PredEdges() const + PredEdgeList PredEdges() const + { + return PredEdgeList(bbPreds); + } + + // PredEdgesEditing: convenience method for enabling range-based `for` iteration over predecessor edges, e.g.: + // for (FlowEdge* const edge : block->PredEdges()) ... + // This iterator tolerates modifications to bbPreds. + // + PredEdgeList PredEdgesEditing() const { - return PredEdgeList(bbPreds); + return PredEdgeList(bbPreds); } // PredBlocks: convenience method for enabling range-based `for` iteration over predecessor blocks, e.g.: @@ -2424,30 +2437,8 @@ inline BasicBlock* BBArrayIterator::operator*() const // Pred list iterator implementations (that are required to be defined after the declaration of BasicBlock and FlowEdge) -inline PredEdgeList::iterator::iterator(FlowEdge* pred) - : m_pred(pred) -{ -#ifdef DEBUG - m_next = (m_pred == nullptr) ? nullptr : m_pred->getNextPredEdge(); -#endif -} - -inline PredEdgeList::iterator& PredEdgeList::iterator::operator++() -{ - FlowEdge* next = m_pred->getNextPredEdge(); - -#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(); -#endif // DEBUG - - m_pred = next; - return *this; -} - -template -inline PredBlockList::iterator::iterator(FlowEdge* pred) +template +inline BasePredIterator::BasePredIterator(FlowEdge* pred) : m_pred(pred) { bool initNextPointer = allowEdits; @@ -2458,14 +2449,8 @@ inline PredBlockList::iterator::iterator(FlowEdge* pred) } } -template -inline BasicBlock* PredBlockList::iterator::operator*() const -{ - return m_pred->getSourceBlock(); -} - -template -inline typename PredBlockList::iterator& PredBlockList::iterator::operator++() +template +inline BasePredIterator& BasePredIterator::operator++() { if (allowEdits) { @@ -2477,11 +2462,9 @@ inline typename PredBlockList::iterator& PredBlockList:: { FlowEdge* next = m_pred->getNextPredEdge(); -#ifdef DEBUG // 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 + INDEBUG(m_next = (m_next == nullptr) ? nullptr : m_next->getNextPredEdge()); m_pred = next; } @@ -2489,6 +2472,12 @@ inline typename PredBlockList::iterator& PredBlockList:: return *this; } +template +inline BasicBlock* PredBlockList::PredBlockIterator::operator*() const +{ + return this->m_pred->getSourceBlock(); +} + /***************************************************************************** * * The following call-backs supplied by the client; it's used by the code From d00f85fb32c74035076458064bf3bff1674696ad Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Thu, 2 Jan 2025 15:57:36 -0500 Subject: [PATCH 2/2] Make operator* non-virtual --- src/coreclr/jit/block.h | 28 +++++++++++++--------------- 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/src/coreclr/jit/block.h b/src/coreclr/jit/block.h index 5e41875e8a4be0..23cf1f4fa44546 100644 --- a/src/coreclr/jit/block.h +++ b/src/coreclr/jit/block.h @@ -205,10 +205,10 @@ struct allMemoryKinds }; // Base class for forward iterators over the predecessor edge linked list. -// Subclasses decide what the iterator should yield (edge, source block, etc.). +// Subclasses decide what the iterator yields (edge, source block, etc.) by implementing the dereference operator. // The pred list cannot be modified during iteration unless allowEdits is true. // -template +template class BasePredIterator { private: @@ -222,11 +222,9 @@ class BasePredIterator protected: FlowEdge* m_pred; -public: BasePredIterator(FlowEdge* pred); - virtual IteratorType* operator*() const = 0; - +public: BasePredIterator& operator++(); bool operator!=(const BasePredIterator& i) const @@ -247,15 +245,15 @@ class PredEdgeList // Forward iterator for the predecessor edges linked list. // - class PredEdgeIterator : public BasePredIterator + class PredEdgeIterator : public BasePredIterator { public: PredEdgeIterator(FlowEdge* pred) - : BasePredIterator(pred) + : BasePredIterator(pred) { } - FlowEdge* operator*() const override + FlowEdge* operator*() const { return this->m_pred; } @@ -290,15 +288,15 @@ class PredBlockList // Forward iterator for the predecessor edges linked list, yielding the predecessor block, not the edge. // - class PredBlockIterator : public BasePredIterator + class PredBlockIterator : public BasePredIterator { public: PredBlockIterator(FlowEdge* pred) - : BasePredIterator(pred) + : BasePredIterator(pred) { } - BasicBlock* operator*() const override; + BasicBlock* operator*() const; }; public: @@ -2437,8 +2435,8 @@ inline BasicBlock* BBArrayIterator::operator*() const // Pred list iterator implementations (that are required to be defined after the declaration of BasicBlock and FlowEdge) -template -inline BasePredIterator::BasePredIterator(FlowEdge* pred) +template +inline BasePredIterator::BasePredIterator(FlowEdge* pred) : m_pred(pred) { bool initNextPointer = allowEdits; @@ -2449,8 +2447,8 @@ inline BasePredIterator::BasePredIterator(FlowEdge* pr } } -template -inline BasePredIterator& BasePredIterator::operator++() +template +inline BasePredIterator& BasePredIterator::operator++() { if (allowEdits) {