[PATCH] Minor improvements to scc_iterator

[Mehdi Amini]

We previously kept two stacks during DFS, with an invariant that s1.size() == s2.size(). Indeed what is really needed is to keep tuples of three elements on a stack, so it is!
Since the guidelines for C++11 are not yet published I assumed that std::tuple is OK. If it is not it can be replaced by a struct{} with operator == defined.
Note also that I replaced a sequence:

if(hash.count(key)) hash[key]=val;

with:

iterator = hash.find(key)
if(iterator != hash.end()) hash[key] = val;

It should save a hash lookup. 

make check is ok on my laptop, with and without assertion enabled.
  Expected Passes    : 9617
  Expected Failures  : 57
  Unsupported Tests  : 34

(that's my first patch submission to LLVM)

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

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,6 +47,19 @@
                         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.
+  typedef std::tuple<NodeType *, ChildItTy, unsigned> StackElement;
+
+  // Indexing in the StackElement tuple
+  enum {
+    NODE = 0,
+    CURRENT_CHILD = 1,
+    MINVAL = 2
+  };
 
   // The visit counters used to detect when a complete SCC is on the stack.
   // visitNum is the global counter.
@@ -59,22 +73,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(std::make_tuple(N, GT::child_begin(N), visitNum));
 #if 0 // Enable if needed when debugging.
     dbgs() << "TarjanSCC: Node " << N <<
           " : visitNum = " << visitNum << "\n";
@@ -84,35 +91,43 @@
   // The stack-based DFS traversal; defined below.
   void DFSVisitChildren() {
     assert(!VisitStack.empty());
-    while (VisitStack.back().second != GT::child_end(VisitStack.back().first)) {
+    while (std::get<CURRENT_CHILD>(VisitStack.back())
+           != GT::child_end(std::get<NODE>(VisitStack.back()))) {
       // TOS has at least one more child so continue DFS
-      NodeType *childN = *VisitStack.back().second++;
-      if (!nodeVisitNumbers.count(childN)) {
+      NodeType *childN = *std::get<CURRENT_CHILD>(VisitStack.back())++;
+      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 (std::get<MINVAL>(VisitStack.back()) > childNum)
+        std::get<MINVAL>(VisitStack.back()) = 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 = std::get<NODE>(VisitStack.back());
+      unsigned minVisitNum = std::get<MINVAL>(VisitStack.back());
+      // DFS Recursion stop when all child are visited
+      assert(std::get<CURRENT_CHILD>(VisitStack.back()) ==
+             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() &&
+          std::get<MINVAL>(VisitStack.back()) > minVisitNum)
+        std::get<MINVAL>(VisitStack.back()) = minVisitNum;
 
 #if 0 // Enable if needed when debugging.
       dbgs() << "TarjanSCC: Popped node " << visitingN <<
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D2672.1.patch
Type: text/x-patch
Size: 4926 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140201/d6d55e3c/attachment.bin>