[PATCH] D79037: [StructurizeCFG] Fix region nodes ordering

[Ehud Katz via Phabricator via llvm-commits]

ekatz marked an inline comment as done.
ekatz added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/StructurizeCFG.cpp:412
+  Order.clear();
+  for (auto SCCI = scc_begin(ParentRegion); !SCCI.isAtEnd(); ++SCCI) {
+    const std::vector<RegionNode *> &SCC = *SCCI;
----------------
sameerds wrote:
> ekatz wrote:
> > sameerds wrote:
> > > Do we really need this for loop? If I understand correctly, orderSCC() works on a subgraph that is guranteed to not be an SCC. Can the same function just take whole region as the starting point?
> > `orderSCC` does something pretty close, but works on `SubGraphNode`s, while this function works on `RegionNode`s. There is no need to create a subgraph if there is no SCC larger than 2, to work on.
> This seems like a good use-case for the WrappedSuccIterator class introduced in LoopIterator.h ... that version limits the graph to the body of a loop, but you could build a similar iterator_adapter that limits the graph to a given set of nodes. With such a succ iterator, you could just eliminate the SubGraphNode class itself and let orderSCC do all the work on the entire input region. That will unify the two for-loops which are actually identical in their intention.
I thought about that, but the problem is with the `scc_iterator` - It created new `succ_iterator` for each child node, using the `GraphTraits` given. This means that the NodeRef, on which `succ_iterator` is created upon, needs to have a reference to the set of valid Nodes (as actual successors).
In any case, we need to create new Nodes.

In addition, please take a look at the implementation of the `RNSuccIterator`, in which you will find that iterating upon it is a pretty heavy operation, including a map search for every node in `getISucc` (this calls to `getNode(BB)` which in turn searches an internal map).

Considering that we create new nodes, and might need to do all over again for each sub SCC, it is better to just rebuild the SCC (with the `SubGraphNode`s), instead of a simple wrapper (that will requery the map for the appropriate `RegionNode` of a `BasicBlock` each time.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79037/new/

https://reviews.llvm.org/D79037