[PATCH] Minor improvements to scc_iterator

[Mehdi Amini]

  I misunderstood Duncan previous comment about renaming, here is the path without any renaming (hopefully)

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

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

Files:
  include/llvm/ADT/SCCIterator.h

Index: include/llvm/ADT/SCCIterator.h
===================================================================
--- include/llvm/ADT/SCCIterator.h
+++ include/llvm/ADT/SCCIterator.h
@@ -46,6 +46,21 @@
                         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.
@@ -59,22 +74,15 @@
   // 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;
     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";
@@ -84,35 +92,41 @@
   // 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);
         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 <<
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D2672.3.patch
Type: text/x-patch
Size: 4770 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140203/d44fe343/attachment.bin>