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

[Ehud Katz via Phabricator via llvm-commits]

ekatz marked 2 inline comments as done.
ekatz added inline comments.


================
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!");
----------------
sameerds wrote:
> 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)?
The nodes in `Nodes` are already connected in the subgraph starting from the `Entry` node. They are all reachable, as they were in the original SCC that contained `Entry` and `Nodes`.
We rewrite the SCC into [OB, OE), which should be the same size as the number of nodes in all the sub-SCCs found in the current `for` loop.


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


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

https://reviews.llvm.org/D79037