[llvm] r300656 - Cleanup some GraphTraits iteration code
Duncan P. N. Exon Smith via llvm-commits
llvm-commits at lists.llvm.org
Wed Dec 6 16:22:40 PST 2017
> On Apr 18, 2017, at 20:22, Tim Shen via llvm-commits <llvm-commits at lists.llvm.org> wrote:
>
> Author: timshen
> Date: Tue Apr 18 22:22:50 2017
> New Revision: 300656
>
> URL: http://llvm.org/viewvc/llvm-project?rev=300656&view=rev
> Log:
> Cleanup some GraphTraits iteration code
>
> Use children<> and nodes<> in appropriate places to cleanup the code.
>
> Also, as part of the cleanup,
> change the signature of DominatorTreeBase's Split.
> It is a protected non-virtual member function called only twice,
> both from within the class,
> and the removed passed argument in both cases is '*this'.
> The reason for the existence of that argument seems to be that
> back before r43115 Split was a free function,
> so an argument to get '*this' was needed - but now that is no longer the
> case.
>
> Patch by Yoav Ben-Shalom!
>
> Differential Revision: https://reviews.llvm.org/D32118
>
> Modified:
> llvm/trunk/include/llvm/Analysis/BlockFrequencyInfoImpl.h
> llvm/trunk/include/llvm/Analysis/DominanceFrontierImpl.h
> llvm/trunk/include/llvm/Analysis/LoopInfo.h
> llvm/trunk/include/llvm/Analysis/LoopInfoImpl.h
> llvm/trunk/include/llvm/Support/GenericDomTree.h
> llvm/trunk/include/llvm/Support/GraphWriter.h
>
> Modified: llvm/trunk/include/llvm/Analysis/BlockFrequencyInfoImpl.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Analysis/BlockFrequencyInfoImpl.h?rev=300656&r1=300655&r2=300656&view=diff
> ==============================================================================
> --- llvm/trunk/include/llvm/Analysis/BlockFrequencyInfoImpl.h (original)
> +++ llvm/trunk/include/llvm/Analysis/BlockFrequencyInfoImpl.h Tue Apr 18 22:22:50 2017
> @@ -1164,9 +1164,8 @@ template <class BT> struct BlockEdgesAdd
> void operator()(IrreducibleGraph &G, IrreducibleGraph::IrrNode &Irr,
> const LoopData *OuterLoop) {
> const BlockT *BB = BFI.RPOT[Irr.Node.Index];
> - for (auto I = Successor::child_begin(BB), E = Successor::child_end(BB);
> - I != E; ++I)
> - G.addEdge(Irr, BFI.getNode(*I), OuterLoop);
> + for (const auto Succ : children<const BlockT *>(BB))
> + G.addEdge(Irr, BFI.getNode(Succ), OuterLoop);
> }
> };
> }
> @@ -1210,10 +1209,9 @@ BlockFrequencyInfoImpl<BT>::propagateMas
> return false;
> } else {
> const BlockT *BB = getBlock(Node);
> - for (auto SI = Successor::child_begin(BB), SE = Successor::child_end(BB);
> - SI != SE; ++SI)
> - if (!addToDist(Dist, OuterLoop, Node, getNode(*SI),
> - getWeightFromBranchProb(BPI->getEdgeProbability(BB, SI))))
> + for (const auto Succ : children<const BlockT *>(BB))
> + if (!addToDist(Dist, OuterLoop, Node, getNode(Succ),
> + getWeightFromBranchProb(BPI->getEdgeProbability(BB, Succ))))
FYI, this resulted in a semantic change and a quadratic compile-time regression, as this changed from passing an edge-iterator to passing a block-reference, a different overload of `getEdgeProbability`. See:
https://reviews.llvm.org/D40891 <https://reviews.llvm.org/D40891>
> // Irreducible backedge.
> return false;
> }
>
> Modified: llvm/trunk/include/llvm/Analysis/DominanceFrontierImpl.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Analysis/DominanceFrontierImpl.h?rev=300656&r1=300655&r2=300656&view=diff
> ==============================================================================
> --- llvm/trunk/include/llvm/Analysis/DominanceFrontierImpl.h (original)
> +++ llvm/trunk/include/llvm/Analysis/DominanceFrontierImpl.h Tue Apr 18 22:22:50 2017
> @@ -174,12 +174,10 @@ ForwardDominanceFrontierBase<BlockT>::ca
> // Visit each block only once.
> if (visited.insert(currentBB).second) {
> // Loop over CFG successors to calculate DFlocal[currentNode]
> - for (auto SI = BlockTraits::child_begin(currentBB),
> - SE = BlockTraits::child_end(currentBB);
> - SI != SE; ++SI) {
> + for (const auto Succ : children<BlockT *>(currentBB)) {
> // Does Node immediately dominate this successor?
> - if (DT[*SI]->getIDom() != currentNode)
> - S.insert(*SI);
> + if (DT[Succ]->getIDom() != currentNode)
> + S.insert(Succ);
> }
> }
>
>
> Modified: llvm/trunk/include/llvm/Analysis/LoopInfo.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Analysis/LoopInfo.h?rev=300656&r1=300655&r2=300656&view=diff
> ==============================================================================
> --- llvm/trunk/include/llvm/Analysis/LoopInfo.h (original)
> +++ llvm/trunk/include/llvm/Analysis/LoopInfo.h Tue Apr 18 22:22:50 2017
> @@ -158,11 +158,8 @@ public:
> /// True if terminator in the block can branch to another block that is
> /// outside of the current loop.
> bool isLoopExiting(const BlockT *BB) const {
> - typedef GraphTraits<const BlockT*> BlockTraits;
> - for (typename BlockTraits::ChildIteratorType SI =
> - BlockTraits::child_begin(BB),
> - SE = BlockTraits::child_end(BB); SI != SE; ++SI) {
> - if (!contains(*SI))
> + for (const auto Succ : children<const BlockT*>(BB)) {
> + if (!contains(Succ))
> return true;
> }
> return false;
> @@ -186,11 +183,8 @@ public:
> unsigned NumBackEdges = 0;
> BlockT *H = getHeader();
>
> - typedef GraphTraits<Inverse<BlockT*> > InvBlockTraits;
> - for (typename InvBlockTraits::ChildIteratorType I =
> - InvBlockTraits::child_begin(H),
> - E = InvBlockTraits::child_end(H); I != E; ++I)
> - if (contains(*I))
> + for (const auto Pred : children<Inverse<BlockT*> >(H))
> + if (contains(Pred))
> ++NumBackEdges;
>
> return NumBackEdges;
> @@ -249,12 +243,9 @@ public:
> /// contains a branch back to the header.
> void getLoopLatches(SmallVectorImpl<BlockT *> &LoopLatches) const {
> BlockT *H = getHeader();
> - typedef GraphTraits<Inverse<BlockT*> > InvBlockTraits;
> - for (typename InvBlockTraits::ChildIteratorType I =
> - InvBlockTraits::child_begin(H),
> - E = InvBlockTraits::child_end(H); I != E; ++I)
> - if (contains(*I))
> - LoopLatches.push_back(*I);
> + for (const auto Pred : children<Inverse<BlockT*>>(H))
> + if (contains(Pred))
> + LoopLatches.push_back(Pred);
> }
>
> //===--------------------------------------------------------------------===//
>
> Modified: llvm/trunk/include/llvm/Analysis/LoopInfoImpl.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Analysis/LoopInfoImpl.h?rev=300656&r1=300655&r2=300656&view=diff
> ==============================================================================
> --- llvm/trunk/include/llvm/Analysis/LoopInfoImpl.h (original)
> +++ llvm/trunk/include/llvm/Analysis/LoopInfoImpl.h Tue Apr 18 22:22:50 2017
> @@ -34,14 +34,11 @@ namespace llvm {
> template<class BlockT, class LoopT>
> void LoopBase<BlockT, LoopT>::
> getExitingBlocks(SmallVectorImpl<BlockT *> &ExitingBlocks) const {
> - typedef GraphTraits<BlockT*> BlockTraits;
> - for (block_iterator BI = block_begin(), BE = block_end(); BI != BE; ++BI)
> - for (typename BlockTraits::ChildIteratorType I =
> - BlockTraits::child_begin(*BI), E = BlockTraits::child_end(*BI);
> - I != E; ++I)
> - if (!contains(*I)) {
> + for (const auto BB : blocks())
> + for (const auto Succ : children<BlockT*>(BB))
> + if (!contains(Succ)) {
> // Not in current loop? It must be an exit block.
> - ExitingBlocks.push_back(*BI);
> + ExitingBlocks.push_back(BB);
> break;
> }
> }
> @@ -63,14 +60,11 @@ BlockT *LoopBase<BlockT, LoopT>::getExit
> template<class BlockT, class LoopT>
> void LoopBase<BlockT, LoopT>::
> getExitBlocks(SmallVectorImpl<BlockT*> &ExitBlocks) const {
> - typedef GraphTraits<BlockT*> BlockTraits;
> - for (block_iterator BI = block_begin(), BE = block_end(); BI != BE; ++BI)
> - for (typename BlockTraits::ChildIteratorType I =
> - BlockTraits::child_begin(*BI), E = BlockTraits::child_end(*BI);
> - I != E; ++I)
> - if (!contains(*I))
> + for (const auto BB : blocks())
> + for (const auto Succ : children<BlockT*>(BB))
> + if (!contains(Succ))
> // Not in current loop? It must be an exit block.
> - ExitBlocks.push_back(*I);
> + ExitBlocks.push_back(Succ);
> }
>
> /// getExitBlock - If getExitBlocks would return exactly one block,
> @@ -88,14 +82,11 @@ BlockT *LoopBase<BlockT, LoopT>::getExit
> template<class BlockT, class LoopT>
> void LoopBase<BlockT, LoopT>::
> getExitEdges(SmallVectorImpl<Edge> &ExitEdges) const {
> - typedef GraphTraits<BlockT*> BlockTraits;
> - for (block_iterator BI = block_begin(), BE = block_end(); BI != BE; ++BI)
> - for (typename BlockTraits::ChildIteratorType I =
> - BlockTraits::child_begin(*BI), E = BlockTraits::child_end(*BI);
> - I != E; ++I)
> - if (!contains(*I))
> + for (const auto BB : blocks())
> + for (const auto Succ : children<BlockT*>(BB))
> + if (!contains(Succ))
> // Not in current loop? It must be an exit block.
> - ExitEdges.push_back(Edge(*BI, *I));
> + ExitEdges.emplace_back(BB, Succ);
> }
>
> /// getLoopPreheader - If there is a preheader for this loop, return it. A
> @@ -134,15 +125,11 @@ BlockT *LoopBase<BlockT, LoopT>::getLoop
>
> // Loop over the predecessors of the header node...
> BlockT *Header = getHeader();
> - typedef GraphTraits<Inverse<BlockT*> > InvBlockTraits;
> - for (typename InvBlockTraits::ChildIteratorType PI =
> - InvBlockTraits::child_begin(Header),
> - PE = InvBlockTraits::child_end(Header); PI != PE; ++PI) {
> - typename InvBlockTraits::NodeRef N = *PI;
> - if (!contains(N)) { // If the block is not in the loop...
> - if (Out && Out != N)
> + for (const auto Pred : children<Inverse<BlockT*>>(Header)) {
> + if (!contains(Pred)) { // If the block is not in the loop...
> + if (Out && Out != Pred)
> return nullptr; // Multiple predecessors outside the loop
> - Out = N;
> + Out = Pred;
> }
> }
>
> @@ -156,17 +143,11 @@ BlockT *LoopBase<BlockT, LoopT>::getLoop
> template<class BlockT, class LoopT>
> BlockT *LoopBase<BlockT, LoopT>::getLoopLatch() const {
> BlockT *Header = getHeader();
> - typedef GraphTraits<Inverse<BlockT*> > InvBlockTraits;
> - typename InvBlockTraits::ChildIteratorType PI =
> - InvBlockTraits::child_begin(Header);
> - typename InvBlockTraits::ChildIteratorType PE =
> - InvBlockTraits::child_end(Header);
> BlockT *Latch = nullptr;
> - for (; PI != PE; ++PI) {
> - typename InvBlockTraits::NodeRef N = *PI;
> - if (contains(N)) {
> + for (const auto Pred : children<Inverse<BlockT*>>(Header)) {
> + if (contains(Pred)) {
> if (Latch) return nullptr;
> - Latch = N;
> + Latch = Pred;
> }
> }
>
> @@ -394,11 +375,9 @@ static void discoverAndMapSubloop(LoopT
> // within this subloop tree itself. Note that a predecessor may directly
> // reach another subloop that is not yet discovered to be a subloop of
> // this loop, which we must traverse.
> - for (typename InvBlockTraits::ChildIteratorType PI =
> - InvBlockTraits::child_begin(PredBB),
> - PE = InvBlockTraits::child_end(PredBB); PI != PE; ++PI) {
> - if (LI->getLoopFor(*PI) != Subloop)
> - ReverseCFGWorklist.push_back(*PI);
> + for (const auto Pred : children<Inverse<BlockT*>>(PredBB)) {
> + if (LI->getLoopFor(Pred) != Subloop)
> + ReverseCFGWorklist.push_back(Pred);
> }
> }
> }
> @@ -482,13 +461,7 @@ analyze(const DominatorTreeBase<BlockT>
> SmallVector<BlockT *, 4> Backedges;
>
> // Check each predecessor of the potential loop header.
> - typedef GraphTraits<Inverse<BlockT*> > InvBlockTraits;
> - for (typename InvBlockTraits::ChildIteratorType PI =
> - InvBlockTraits::child_begin(Header),
> - PE = InvBlockTraits::child_end(Header); PI != PE; ++PI) {
> -
> - BlockT *Backedge = *PI;
> -
> + for (const auto Backedge : children<Inverse<BlockT*>>(Header)) {
> // If Header dominates predBB, this is a new loop. Collect the backedges.
> if (DomTree.dominates(Header, Backedge)
> && DomTree.isReachableFromEntry(Backedge)) {
>
> Modified: llvm/trunk/include/llvm/Support/GenericDomTree.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/GenericDomTree.h?rev=300656&r1=300655&r2=300656&view=diff
> ==============================================================================
> --- llvm/trunk/include/llvm/Support/GenericDomTree.h (original)
> +++ llvm/trunk/include/llvm/Support/GenericDomTree.h Tue Apr 18 22:22:50 2017
> @@ -276,32 +276,25 @@ protected:
>
> // NewBB is split and now it has one successor. Update dominator tree to
> // reflect this change.
> - template <class N, class GraphT>
> - void Split(DominatorTreeBaseByGraphTraits<GraphT> &DT,
> - typename GraphT::NodeRef NewBB) {
> + template <class N>
> + void Split(typename GraphTraits<N>::NodeRef NewBB) {
> + using GraphT = GraphTraits<N>;
> + using NodeRef = typename GraphT::NodeRef;
> assert(std::distance(GraphT::child_begin(NewBB),
> GraphT::child_end(NewBB)) == 1 &&
> "NewBB should have a single successor!");
> - typename GraphT::NodeRef NewBBSucc = *GraphT::child_begin(NewBB);
> + NodeRef NewBBSucc = *GraphT::child_begin(NewBB);
>
> - std::vector<typename GraphT::NodeRef> PredBlocks;
> - typedef GraphTraits<Inverse<N>> InvTraits;
> - for (typename InvTraits::ChildIteratorType
> - PI = InvTraits::child_begin(NewBB),
> - PE = InvTraits::child_end(NewBB);
> - PI != PE; ++PI)
> - PredBlocks.push_back(*PI);
> + std::vector<NodeRef> PredBlocks;
> + for (const auto Pred : children<Inverse<N>>(NewBB))
> + PredBlocks.push_back(Pred);
>
> assert(!PredBlocks.empty() && "No predblocks?");
>
> bool NewBBDominatesNewBBSucc = true;
> - for (typename InvTraits::ChildIteratorType
> - PI = InvTraits::child_begin(NewBBSucc),
> - E = InvTraits::child_end(NewBBSucc);
> - PI != E; ++PI) {
> - typename InvTraits::NodeRef ND = *PI;
> - if (ND != NewBB && !DT.dominates(NewBBSucc, ND) &&
> - DT.isReachableFromEntry(ND)) {
> + for (const auto Pred : children<Inverse<N>>(NewBBSucc)) {
> + if (Pred != NewBB && !dominates(NewBBSucc, Pred) &&
> + isReachableFromEntry(Pred)) {
> NewBBDominatesNewBBSucc = false;
> break;
> }
> @@ -312,7 +305,7 @@ protected:
> NodeT *NewBBIDom = nullptr;
> unsigned i = 0;
> for (i = 0; i < PredBlocks.size(); ++i)
> - if (DT.isReachableFromEntry(PredBlocks[i])) {
> + if (isReachableFromEntry(PredBlocks[i])) {
> NewBBIDom = PredBlocks[i];
> break;
> }
> @@ -324,18 +317,18 @@ protected:
> return;
>
> for (i = i + 1; i < PredBlocks.size(); ++i) {
> - if (DT.isReachableFromEntry(PredBlocks[i]))
> - NewBBIDom = DT.findNearestCommonDominator(NewBBIDom, PredBlocks[i]);
> + if (isReachableFromEntry(PredBlocks[i]))
> + NewBBIDom = findNearestCommonDominator(NewBBIDom, PredBlocks[i]);
> }
>
> // Create the new dominator tree node... and set the idom of NewBB.
> - DomTreeNodeBase<NodeT> *NewBBNode = DT.addNewBlock(NewBB, NewBBIDom);
> + DomTreeNodeBase<NodeT> *NewBBNode = addNewBlock(NewBB, NewBBIDom);
>
> // If NewBB strictly dominates other blocks, then it is now the immediate
> // dominator of NewBBSucc. Update the dominator tree as appropriate.
> if (NewBBDominatesNewBBSucc) {
> - DomTreeNodeBase<NodeT> *NewBBSuccNode = DT.getNode(NewBBSucc);
> - DT.changeImmediateDominator(NewBBSuccNode, NewBBNode);
> + DomTreeNodeBase<NodeT> *NewBBSuccNode = getNode(NewBBSucc);
> + changeImmediateDominator(NewBBSuccNode, NewBBNode);
> }
> }
>
> @@ -379,7 +372,7 @@ public:
> if (DomTreeNodes.size() != OtherDomTreeNodes.size())
> return true;
>
> - for (const auto &DomTreeNode : this->DomTreeNodes) {
> + for (const auto &DomTreeNode : DomTreeNodes) {
> NodeT *BB = DomTreeNode.first;
> typename DomTreeNodeMapType::const_iterator OI =
> OtherDomTreeNodes.find(BB);
> @@ -663,10 +656,9 @@ public:
> /// tree to reflect this change.
> void splitBlock(NodeT *NewBB) {
> if (this->IsPostDominators)
> - this->Split<Inverse<NodeT *>, GraphTraits<Inverse<NodeT *>>>(*this,
> - NewBB);
> + Split<Inverse<NodeT *>>(NewBB);
> else
> - this->Split<NodeT *, GraphTraits<NodeT *>>(*this, NewBB);
> + Split<NodeT *>(NewBB);
> }
>
> /// print - Convert to human readable form
> @@ -677,7 +669,7 @@ public:
> o << "Inorder PostDominator Tree: ";
> else
> o << "Inorder Dominator Tree: ";
> - if (!this->DFSInfoValid)
> + if (!DFSInfoValid)
> o << "DFSNumbers invalid: " << SlowQueries << " slow queries.";
> o << "\n";
>
> @@ -712,12 +704,12 @@ protected:
> // immediate dominator.
> NodeT *IDom = getIDom(BB);
>
> - assert(IDom || this->DomTreeNodes[nullptr]);
> + assert(IDom || DomTreeNodes[nullptr]);
> DomTreeNodeBase<NodeT> *IDomNode = getNodeForBlock(IDom);
>
> // Add a new tree node for this NodeT, and link it as a child of
> // IDomNode
> - return (this->DomTreeNodes[BB] = IDomNode->addChild(
> + return (DomTreeNodes[BB] = IDomNode->addChild(
> llvm::make_unique<DomTreeNodeBase<NodeT>>(BB, IDomNode))).get();
> }
>
> @@ -780,7 +772,7 @@ public:
> template <class FT> void recalculate(FT &F) {
> typedef GraphTraits<FT *> TraitsTy;
> reset();
> - this->Vertex.push_back(nullptr);
> + Vertex.push_back(nullptr);
>
> if (!this->IsPostDominators) {
> // Initialize root
>
> Modified: llvm/trunk/include/llvm/Support/GraphWriter.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/GraphWriter.h?rev=300656&r1=300655&r2=300656&view=diff
> ==============================================================================
> --- llvm/trunk/include/llvm/Support/GraphWriter.h (original)
> +++ llvm/trunk/include/llvm/Support/GraphWriter.h Tue Apr 18 22:22:50 2017
> @@ -143,10 +143,9 @@ public:
>
> void writeNodes() {
> // Loop over the graph, printing it out...
> - for (node_iterator I = GTraits::nodes_begin(G), E = GTraits::nodes_end(G);
> - I != E; ++I)
> - if (!isNodeHidden(*I))
> - writeNode(*I);
> + for (const auto Node : nodes<GraphType>(G))
> + if (!isNodeHidden(Node))
> + writeNode(Node);
> }
>
> bool isNodeHidden(NodeRef Node) {
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20171206/50a88dd2/attachment.html>
More information about the llvm-commits
mailing list