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

[Ehud Katz via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/Transforms/Scalar/StructurizeCFG.cpp:327
+/// Insert to the WorkList all the ancestors of L up to Ancestor.
+static bool pushPathToAncestor(Loop *L, Loop *Ancestor,
+                               SmallDenseMap<Loop *, unsigned, 8> &LoopSizes,
----------------
sameerds wrote:
> Wouldn't this be more readable as a simple iteration with a temporary vector? The temporary vector is no more expensive than the stack created for the recursive calls. Essentially:
> 
> 
> ```
> while (L && L != Ancestor) {
>   push back L to temporary vector;
>   L = parent;
> }
> 
> if (!L) return false; // Ancestor is not an ancestor of L
> 
> pop temporary vector to WorkList
> return true;
> ```
I found a nice iterative (non-recursive) way to write this function. I'll change to that.


================
Comment at: llvm/lib/Transforms/Scalar/StructurizeCFG.cpp:367
+  auto LSI = LoopSizes.find(nullptr);
+  unsigned *CurrentLoopSize = LSI != LoopSizes.end() ? &LSI->second : nullptr;
   Loop *CurrentLoop = nullptr;
----------------
sameerds wrote:
> This pointer is never checked for nullptr before it is used. So either assert that it is not null, or use a reference instead. Also what does the previous line mean? What is the size of a nullptr loop?
I'll add some comments, and an assert.


================
Comment at: llvm/lib/Transforms/Scalar/StructurizeCFG.cpp:378
+  auto O = Order.rbegin(), OE = Order.rend();
+  while (O != OE) {
+    // If we have any skipped nodes, then erase the gap between the end of the
----------------
sameerds wrote:
> Isn't this the same as `while (!POT.empty())`? That would be more readable, and also convey the fact that a node from POT is definitely consumed on each iteration.
`POT` is not always get emptied. If there are no **skipped** nodes there is no need to erase anything, and we do not enter the following `if` statement.


================
Comment at: llvm/lib/Transforms/Scalar/StructurizeCFG.cpp:393
+    // Keep processing loops while only going deeper (into inner loops).
+    do {
+      assert(I != POT.rend());
----------------
sameerds wrote:
> Why not just have `while (CurrentLoopSize)` here? At least from what I could see, the value should be non-zero when this point is reached from above, right?
The problem is that `CurrentLoopSize` may be null in case all of the nodes have a parent loop (none outside from every loop).
In that case, `CurrentLoopSize` will only be valid from line 408; and remain that way all the way until the end.


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

https://reviews.llvm.org/D79037