[PATCH] Minor improvements to scc_iterator

[Duncan P. N. Exon Smith]

On 2014 Feb 1, at 15:56, Mehdi Amini <mehdi.amini at silkan.com> wrote:

> 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.

Thanks for the patch!

I think a simple struct will give cleaner code than what you have right now
(I don’t know what the guidelines are either, but that’s beside the point).
I have some examples inline.

> 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. 

Excellent find!

> 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
> +  };

I think this code is more direct:

  struct StackElement {
    NodeType *Node;
    ChildItTy Child;
    unsigned Min;
  };

It’s not too laborious to add the extra functions you need (like a constructor).

> 
>   // 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()))) {

Again, this looks better:

      while (VisitStack.back().Child != 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 = *std::get<CURRENT_CHILD>(VisitStack.back())++;
> +      auto visited = nodeVisitNumbers.find(childN);

“Visited" (uppercase) is better LLVM style than “visited".  This function isn’t
terribly consistent, but your new variable may as well match standard style.

> +      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 <<
> <D2672.1.patch>_______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits