[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:
> > > 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.
> I didn't follow the part about creating new nodes. The LoopIterator defines the NodeRef as a tuple <BasicBlock, Loop>, so that it carries the Loop around for use in succ_iterator. What I had in mind was a similar tuple <RegionNode, ValidNodes> where the second part is a collection of nodes that constrain the succ_iterator.
> 
> I would choose a more compact implementation here instead of looking into the cost of RNSuccIterator. Avoiding the map does not look like sufficient reason to break away from the idiomatic use of GraphTraits and scc_iterator. I know this looks expensive, but how expensive? Something like O(N * scc_depth)?  It is certainly dwarfed by the heavy lifting that the rest of the structurizer is doing.
> 
> To clarify, I am not insisting that a new iterator is ideal, but it would certainly be my choice unless there is peformance data asking for improvement.
I didn't like the concept of carrying a reference to the "set" on each node, which will increase the memory size required by the SCC iterator by a factor of 2, and also require requery of possibly many nodes (in the "set").

However, I agree that the performance impact overall is probably negligible, and we can always get back to a more optimized implementation if needed. But for now, the simpler approach is probably the correct one.

I will upload a version without building a subgraph, as suggested.


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

https://reviews.llvm.org/D79037