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

[Ehud Katz via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/Transforms/Scalar/StructurizeCFG.cpp:151
+    auto I = LoopSizes.find(L);
+    return I != LoopSizes.end() ? &I->second : nullptr;
+  }
----------------
sameerds wrote:
> 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.
Because I need to update the value later.
In any case, I will remove this function, as it can be simplified.


================
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.
----------------
sameerds wrote:
> Should this comment be removed now? The problem with the backedges of the outer loops is addressed in this change, right?
It is still true, it should be treated as an intro for the following algorithm (explaining why we don't stop here).
I'll add a sentence to make it clearer.


================
Comment at: llvm/lib/Transforms/Scalar/StructurizeCFG.cpp:388-390
+  while (true) {
+    // Keep processing loops while only going deeper (into inner loops).
+    while (true) {
----------------
sameerds wrote:
> 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?
I'll rearrange those loops.


================
Comment at: llvm/lib/Transforms/Scalar/StructurizeCFG.cpp:443
+  RegionSorter RS(LI, Order);
+  RS.run(ParentRegion);
 }
----------------
sameerds wrote:
> 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.
I can reduce it to a single utility function, and then it will make even less sense to keep it. Then I'll remove it.


================
Comment at: llvm/test/Transforms/StructurizeCFG/pr41703.ll:4
+
+; This test used to produce incorrect code.
+
----------------
sameerds wrote:
> 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.
sure


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

https://reviews.llvm.org/D79037