[PATCH] D44199: [LoopRotate] Attempt to Rotate loops if it can lead to removing phi nodes.

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: lib/Transforms/Scalar/LoopRotation.cpp:181
+    if (llvm::any_of(Phi.users(), [L](const User *U) {
+          return !isa<PHINode>(U) || (isa<Instruction>(U) &&
+                                      L->contains(cast<const Instruction>(U)));
----------------
efriedma wrote:
> A user of an Instruction is always an Instruction, so the isa<> is redundant.
> 
> This heuristic doesn't seem very general; it's not clear to me why we specifically want to rotate in cases like this, as opposed to peeling or something like that.
Sorry for the delay.

By "not very general", do you mean that this won't catch loops that should be rotated?

Or that this will rotate loops that shouldn't be rotated?

The benchmarks I ran didn't show any regressions from this patch (for what that's worth :) ). I'm just re-running them now to make sure that's still true.

This case is a sorted list insert which I'm trying to come up with a sensible heuristic for. I'll admit I don't think I fully understand why the old heuristic worked (if we remove a latch, then rotate the loop). But at least in this case it did help, so am attempting come up with something that has the same affect.


https://reviews.llvm.org/D44199