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

[Sameer Sahasrabuddhe via Phabricator via llvm-commits]

sameerds added a comment.

This needs more tests. There are a bunch of decisions being taken during the reordering, but the test peresented simply exercises the case of one nested loop inside another. For example, what about two inner loops L1 <https://reviews.llvm.org/L1> and L2, where L1 <https://reviews.llvm.org/L1> has an exit that reaches the entry of L2? There might be other interesting cases too.

We actually introduced a different pass recently, called "UnifyLoopExits" to circumvent the same problem in the structurizer. You can see it optionally invoked in the AMDGPU backend. The idea is to transform each loop to have a single exit block, which makes it look like a separate region; since the structurizer is a region pass, it no longer sees a loop nest and thus cannot get confused about the nesting.

The current patch can provide a different, more precise way to solve the same problem. But please do take a look at the tests that use UnifyLoopExits. They are under test/Transforms/UnifyLoopExits and test/Transforms/StructurizeCFG/workarounds.



================
Comment at: llvm/lib/Transforms/Scalar/StructurizeCFG.cpp:151
+    auto I = LoopSizes.find(L);
+    return I != LoopSizes.end() ? &I->second : nullptr;
+  }
----------------
Why return a pointer here? As far as I could see, the return value is never checked for nullptr in the places where this function is called.


================
Comment at: llvm/lib/Transforms/Scalar/StructurizeCFG.cpp:378-380
+  // The post-order traversal of the list gives us an ordering close to what we
+  // want.  The only problem with it is that sometimes backedges for outer loops
+  // will be visited before backedges for inner loops.
----------------
Should this comment be removed now? The problem with the backedges of the outer loops is addressed in this change, right?


================
Comment at: llvm/lib/Transforms/Scalar/StructurizeCFG.cpp:388-390
+  while (true) {
+    // Keep processing loops while only going deeper (into inner loops).
+    while (true) {
----------------
It's very hard to read these two nested loops, especially with respect to their exit conditions. If I understand correctly, the inner loop at line 390 keeps executing until the correct number of blocks is found for a given loop. Can this condition be placed in the while () itself?


================
Comment at: llvm/lib/Transforms/Scalar/StructurizeCFG.cpp:443
+  RegionSorter RS(LI, Order);
+  RS.run(ParentRegion);
 }
----------------
It seems the only purpose of this object is to invoke the "run()" method. Could this be just split out into a bunch of functions instead? All the class members can be declared right here, and the member functions of the current class can be rewritten to take necessary structures as arguments.


================
Comment at: llvm/test/Transforms/StructurizeCFG/pr41703.ll:4
+
+; This test used to produce incorrect code.
+
----------------
Can you please add a comment indicating which blocks end up in the wrong order? This will be useful when revisiting this testcase at some future time.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79037