[PATCH] D22630: Loop rotation

[Aditya Kumar via llvm-commits]

hiraditya added a comment.

In https://reviews.llvm.org/D22630#491624, @eli.friedman wrote:

> It looks like you're adding a bunch of bugpoint-reduced testcases without any comments... tests should at least explain what specifically they're trying to test.
>
>  ----
>
> This might be easier to review if it were split up into multiple patches, something like:
>
> 1. Split checking whether rotation is legal/profitable into a separate function.
> 2. Replace the actual transform code with your generalized version.
> 3. Change the rules for when rotation is legal/profitable given the more flexible implementation.
>
>   As it is, you're mixing up a bunch of different changes to the way rotation works, so it's hard to tell whether certain changes are intentional.


I'll try to split up into multiple patches and add comments to test cases as well.
Thanks for the review.


================
Comment at: llvm/include/llvm/Transforms/Utils/Cloning.h:238-239
@@ +237,4 @@
+/// and post-dominated by \p Exit.
+bool isSESE(const BasicBlock *Entry, const BasicBlock *Exit, DominatorTree *DT,
+            DominatorTree *PDT);
+
----------------
Meinersbur wrote:
> I don't see any use of `isSESE()`. What is it good for?
True, this isn't used anywhere. Since I was writing isSEME anyways, I added this just to provide this functionality in case it might be useful to some other users. I can remove this if this doesn't appear useful.

================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:149
@@ -202,2 +148,3 @@
+  const BasicBlock *OrigHeader = L->getHeader();
   if (!L->isLoopExiting(OrigHeader))
     return false;
----------------
eli.friedman wrote:
> Did you mean to change this check? Given the way you've implemented cloning, you should be able to do something more aggressive.
The new implementation can rotate the loop as far as user wants. This check is here because currently we copy until we find a basic block which is not exiting. I guess if there are use cases where a very general copying is helpful, we can remove this check.

================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:154
@@ -213,2 +153,3 @@
+  if (!BI || BI->isUnconditional())
     return false;
 
----------------
eli.friedman wrote:
> Why does this matter?  (At the very least, needs a comment.)
Seems like this was leftover from the previous implementation. It does not appear useful now that you mentioned it. I'll investigate more and update on this.

================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:162
@@ +161,3 @@
+  const BasicBlock *LoopLatch = L->getLoopLatch();
+  if (!isSEME(OrigHeader, LoopLatch, DT))
+    return false;
----------------
eli.friedman wrote:
> Isn't a loop an SEME region by definition?
If the loop is infinite then I'm not sure. Also if the loop-body has incoming branches from outside of region (irreducible control flow), then the loop won't be an SEME. Although I'm not sure if llvm loop-info will detect such loops in the first place. I'll investigate.

================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:202
@@ +201,3 @@
+    else
+      return false;
+  }
----------------
eli.friedman wrote:
> This is not the way to count the size of a loop... see the existing code using CodeMetrics.  (In particular, you can never use size() as a threshold for anything because it's affected by debug info.)
Will change that. Thanks

================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:327
@@ +326,3 @@
+
+  // Remove this code.
+  BasicBlock *BeforeLoop = nullptr;
----------------
eli.friedman wrote:
> ?
Some leftover comment. I'll fix this. Thanks,

================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:431
@@ +430,3 @@
+    DEBUG(dbgs() << "\nSplitting the edge of Loop:"; L->dumpVerbose(););
+    LoopLatch = SplitEdge(LoopLatch, L->getHeader(), DT, LI);
+  }
----------------
eli.friedman wrote:
> The loop latch could be an indirectbr.
I'll fix this, thanks.

================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:553
@@ -552,3 @@
-/// I don't believe this invalidates SCEV.
-bool LoopRotate::simplifyLoopLatch(Loop *L) {
-  BasicBlock *Latch = L->getLoopLatch();
----------------
eli.friedman wrote:
> Why are you getting rid of this?
It removes loop-latch and leaves the loop in non-canonical form (latch should have only one successor).


https://reviews.llvm.org/D22630