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

[Sameer Sahasrabuddhe via Phabricator via llvm-commits]

sameerds added a comment.

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

> In D79037#2016160 <https://reviews.llvm.org/D79037#2016160>, @sameerds wrote:
>
> > It seems the new commit is an incremental update on the previous commit. Can you please submit a single squashed commit, so that the change can be compared against the original state of the trunk?
>
>
> I am not sure what do you mean. In this page in the Phabricator, under [Revision Contents]->[History] you can choose which of the patch revisions to diff. The default is "Base" against the latest "Diff no.".


Sorry about that! I can see the diff now ...

> 
> 
>> Also, the following tests were created specifically to demonstrate the wrong ordering:
>> 
>> - test/Transforms/StructurizeCFG/workarounds/needs-unified-loop-exits.ll
>> - test/Transforms/StructurizeCFG/workarounds/needs-fr-ule.ll
> 
> Great! Didn't know about those. I'll update them, and if everything is OK, I'll move them out of the "workarounds" directory.

Note that needs-fr-ule.ll is quite a painful CFG from a real world testcase. It needs FixIrreducible pass, which produces extra loops that have the same ordering problem being fixed here. I'll try to run your patch myself and point out which blocks show up in the wrong order.

>> If the proposed change is correct, then the example in these tests will be structurized correctly even after removing the "-unify-loop-exits" argument. These tests should also be checked and then updated accordingly.
>> 
>> In fact, it would be nice to update the following tests. But they have a very confusing history and they are specific to AMDGPU. I can volunteer to do this later as a follow up.
>> 
>> - test/Transforms/StructurizeCFG/AMDGPU/backedge-id-bug.ll
>> - test/Transforms/StructurizeCFG/AMDGPU/backedge-id-bug-xfail.ll
> 
> I'll try to take a look at those as well. These are "XFAIL" right now. If I'm understanding correctly, You assume they should pass with this patch?

That would be great! But like I said, the file history is confusing, and the last time that I looked, I was unable to decide which blocks are in the wrong order.



================
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,
----------------
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;
```


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


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


================
Comment at: llvm/lib/Transforms/Scalar/StructurizeCFG.cpp:393
+    // Keep processing loops while only going deeper (into inner loops).
+    do {
+      assert(I != POT.rend());
----------------
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?


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

https://reviews.llvm.org/D79037