[PATCH] Minor improvements to scc_iterator

[Mehdi Amini]

  Update, thanks to review from Duncan P. N. Exon Smith:

  - naming convention
  - replaced std::tuple with a struct.

  I noticed a minor semantical change with respect to the original code: iterator equality is implemented the following way:

    inline bool operator==(const scc_iterator &x) const {
      return VisitStack == x.VisitStack && CurrentSCC == x.CurrentSCC;
    }

  Previously the MinVisited value was stored on a separated stack and not checked for equality, now it is.

http://llvm-reviews.chandlerc.com/D2672

CHANGE SINCE LAST DIFF
  http://llvm-reviews.chandlerc.com/D2672?vs=6819&id=6820#toc

Files:
  include/llvm/ADT/SCCIterator.h

Index: include/llvm/ADT/SCCIterator.h
===================================================================
--- include/llvm/ADT/SCCIterator.h
+++ include/llvm/ADT/SCCIterator.h
@@ -25,6 +25,7 @@
 
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/GraphTraits.h"
+#include <tuple>
 #include <vector>
 
 namespace llvm {
@@ -46,81 +47,95 @@
                         std::vector<typename GT::NodeType>, ptrdiff_t> super;
   typedef typename super::reference reference;
   typedef typename super::pointer pointer;
+  // Type for elements kept on VisitStack during DFS
+  // The first element is the current node pointer
+  // The second is the 'next child' to visit, it is modified inplace during DFS
+  // The last element is the "minValue", i.e. it identify the node below on the
+  // stack that delimit the current SCC.
+  struct StackElement {
+    NodeType *Node;
+    ChildItTy CurrentChild;
+    unsigned MinVisited;
+    bool operator==(const StackElement &other) const {
+      return Node == other.Node &&
+             CurrentChild == other.CurrentChild &&
+             MinVisited == other.MinVisited;
+    }
+  };
 
   // The visit counters used to detect when a complete SCC is on the stack.
   // visitNum is the global counter.
   // nodeVisitNumbers are per-node visit numbers, also used as DFS flags.
-  unsigned visitNum;
-  DenseMap<NodeType *, unsigned> nodeVisitNumbers;
+  unsigned VisitNum;
+  DenseMap<NodeType *, unsigned> NodeVisitNumbers;
 
   // Stack holding nodes of the SCC.
   std::vector<NodeType *> SCCNodeStack;
 
   // The current SCC, retrieved using operator*().
   SccTy CurrentSCC;
 
-  // Used to maintain the ordering.  The top is the current block, the first
-  // element is basic block pointer, second is the 'next child' to visit.
-  std::vector<std::pair<NodeType *, ChildItTy> > VisitStack;
-
-  // Stack holding the "min" values for each node in the DFS. This is used to
-  // track the minimum uplink values for all children of the corresponding node
-  // on the VisitStack.
-  std::vector<unsigned> MinVisitNumStack;
+  // Used to maintain the ordering. The top is the current node
+  std::vector<StackElement> VisitStack;
 
   // A single "visit" within the non-recursive DFS traversal.
   void DFSVisitOne(NodeType *N) {
-    ++visitNum;
-    nodeVisitNumbers[N] = visitNum;
+    ++VisitNum;
+    NodeVisitNumbers[N] = VisitNum;
     SCCNodeStack.push_back(N);
-    MinVisitNumStack.push_back(visitNum);
-    VisitStack.push_back(std::make_pair(N, GT::child_begin(N)));
+    VisitStack.push_back({N, GT::child_begin(N), VisitNum});
 #if 0 // Enable if needed when debugging.
     dbgs() << "TarjanSCC: Node " << N <<
-          " : visitNum = " << visitNum << "\n";
+          " : VisitNum = " << VisitNum << "\n";
 #endif
   }
 
   // The stack-based DFS traversal; defined below.
   void DFSVisitChildren() {
     assert(!VisitStack.empty());
-    while (VisitStack.back().second != GT::child_end(VisitStack.back().first)) {
+    while (VisitStack.back().CurrentChild !=
+           GT::child_end(VisitStack.back().Node)) {
       // TOS has at least one more child so continue DFS
-      NodeType *childN = *VisitStack.back().second++;
-      if (!nodeVisitNumbers.count(childN)) {
+      NodeType *ChildN = *VisitStack.back().CurrentChild++;
+      auto Visited = NodeVisitNumbers.find(ChildN);
+      if (Visited == NodeVisitNumbers.end()) {
         // this node has never been seen.
-        DFSVisitOne(childN);
+        DFSVisitOne(ChildN);
         continue;
       }
 
-      unsigned childNum = nodeVisitNumbers[childN];
-      if (MinVisitNumStack.back() > childNum)
-        MinVisitNumStack.back() = childNum;
+      unsigned ChildNum = Visited->second;
+      if (VisitStack.back().MinVisited > ChildNum)
+        VisitStack.back().MinVisited = ChildNum;
     }
   }
 
   // Compute the next SCC using the DFS traversal.
   void GetNextSCC() {
-    assert(VisitStack.size() == MinVisitNumStack.size());
     CurrentSCC.clear(); // Prepare to compute the next SCC
     while (!VisitStack.empty()) {
       DFSVisitChildren();
-      assert(VisitStack.back().second ==
-             GT::child_end(VisitStack.back().first));
-      NodeType *visitingN = VisitStack.back().first;
-      unsigned minVisitNum = MinVisitNumStack.back();
+      // Back from DFSVisitChildren with a leaf on top of the VisitStack, pop it
+      NodeType *VisitingN = VisitStack.back().Node;
+      unsigned MinVisitNum = VisitStack.back().MinVisited;
+      // DFS Recursion stop when all child are visited
+      assert(VisitStack.back().CurrentChild == GT::child_end(VisitingN));
       VisitStack.pop_back();
-      MinVisitNumStack.pop_back();
-      if (!MinVisitNumStack.empty() && MinVisitNumStack.back() > minVisitNum)
-        MinVisitNumStack.back() = minVisitNum;
+
+      // If we found a SCC (loop in the graph), then minVisitNum for the leaf
+      // may be lower than its predecessor in the DFS path, propagate it back
+      // this is required because we detect the starting node of the SCC when
+      // minVisitNum is unchanged.
+      if (!VisitStack.empty() && VisitStack.back().MinVisited > MinVisitNum)
+        VisitStack.back().MinVisited = MinVisitNum;
 
 #if 0 // Enable if needed when debugging.
-      dbgs() << "TarjanSCC: Popped node " << visitingN <<
-            " : minVisitNum = " << minVisitNum << "; Node visit num = " <<
-            nodeVisitNumbers[visitingN] << "\n";
+      dbgs() << "TarjanSCC: Popped node " << VisitingN <<
+            " : minVisitNum = " << MinVisitNum << "; Node visit num = " <<
+            NodeVisitNumbers[VisitingN] << "\n";
 #endif
 
-      if (minVisitNum != nodeVisitNumbers[visitingN])
+      if (MinVisitNum != NodeVisitNumbers[VisitingN])
         continue;
 
       // A full SCC is on the SCCNodeStack!  It includes all nodes below
@@ -130,14 +145,14 @@
       do {
         CurrentSCC.push_back(SCCNodeStack.back());
         SCCNodeStack.pop_back();
-        nodeVisitNumbers[CurrentSCC.back()] = ~0U;
-      } while (CurrentSCC.back() != visitingN);
+        NodeVisitNumbers[CurrentSCC.back()] = ~0U;
+      } while (CurrentSCC.back() != VisitingN);
       return;
     }
   }
 
-  inline scc_iterator(NodeType *entryN) : visitNum(0) {
-    DFSVisitOne(entryN);
+  inline scc_iterator(NodeType *EntryN) : VisitNum(0) {
+    DFSVisitOne(EntryN);
     GetNextSCC();
   }
 
@@ -200,9 +215,9 @@
   /// This informs the \c scc_iterator that the specified \c Old node
   /// has been deleted, and \c New is to be used in its place.
   void ReplaceNode(NodeType *Old, NodeType *New) {
-    assert(nodeVisitNumbers.count(Old) && "Old not in scc_iterator?");
-    nodeVisitNumbers[New] = nodeVisitNumbers[Old];
-    nodeVisitNumbers.erase(Old);
+    assert(NodeVisitNumbers.count(Old) && "Old not in scc_iterator?");
+    NodeVisitNumbers[New] = NodeVisitNumbers[Old];
+    NodeVisitNumbers.erase(Old);
   }
 };
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D2672.2.patch
Type: text/x-patch
Size: 6932 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140201/9ecd434a/attachment.bin>