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

[Sameer Sahasrabuddhe via Phabricator via llvm-commits]

sameerds added a comment.

In D79037#2054462 <https://reviews.llvm.org/D79037#2054462>, @ekatz wrote:

> This is a second take of the solution, using SCCs instead of LoopInfo.
>  This algorithm can also handle irreducible loops, and, generally, any kind of cycle in the graph.


Thanks! This looks really nice now! The sad part is that the structurizer itself is undefined in the presence of irreducible regions, so we will never actually see this in action around irreducible regions.



================
Comment at: llvm/lib/Transforms/Scalar/StructurizeCFG.cpp:373
+      // Add the SCC nodes to the Out array.
+      for (SubGraphNode *N : SCC) {
+        assert(OB < OE && "SCC size mismatch!");
----------------
I might be missing something here. OB marks the beginning of the range that originally contained the nodes that are now listed in Nodes. We are now over-writing this range from the beginning with members of each SCC found by the scc_iterator. Where do the rest of the members in Nodes go? Do they have to be rewritten in the remaining range [OB, OE)?


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


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

https://reviews.llvm.org/D79037