[PATCH] Minor improvements to scc_iterator

[Duncan P. N. Exon Smith]

On Feb 3, 2014, at 7:35 PM, Mehdi Amini <mehdi.amini at silkan.com> wrote:

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

Thanks for the resubmit.

LGTM, after you solve a few final nitpicks (mainly comments).

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

I’d add a newline here.

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

CurrentChild => NextChild

> +    unsigned MinVisited;
> +    bool operator==(const StackElement &other) const {
> +      return Node == other.Node &&
> +             CurrentChild == other.CurrentChild &&
> +             MinVisited == other.MinVisited;
> +    }
> +  };

The comments here should be inline, and be full sentences.  Also, “other” should
start with a capital.  Something like:

  /// Element of VisitStack during DFS.
  struct StackElement {
    NodeType *Node; ///< The current node pointer.
    ChildItTy NextChild; ///< The next child.
    unsigned MinVisited; ///< Minimum uplink value of all children of Node.
    bool operator==(const StackElement &Other) const {
      return Node == Other.Node &&
             NextChild == Other.NextChild &&
             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

The old comments were more informative, and used complete sentences.

  // Used to maintain the ordering.  The top contains the current node, the
  // next child to visit, and the minimum uplink value of all children.

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

I’m surprised, but from recent traffic on llvm-commits, it looks like C++11 is
still not allowed.  You have two options:

1. Sit on this until C++11 is allowed.
2. Write a constructor for StackElement and use it.

I don’t think you’d have to wait too long, but there’s no guarantee.

> #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 !=

CurrentChild => NextChild

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

CurrentChild => NextChild

> +      auto Visited = nodeVisitNumbers.find(childN);

Same deal here.  Either wait for C++11 to be okay or don’t use auto.

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

CurrentChild => NextChild

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

This comment isn’t clear, mostly because of sentence structure.   I think the
following is enough, though:

      // Propagate MinVisitNum so we can detect the SCC starting node.

> +      if (!VisitStack.empty() && VisitStack.back().MinVisited > minVisitNum)
> +        VisitStack.back().MinVisited = minVisitNum;
> 
> #if 0 // Enable if needed when debugging.
>       dbgs() << "TarjanSCC: Popped node " << visitingN <<
> <D2672.3.patch>_______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits