[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:
> dmgreen wrote:
> > 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.
> By "not very general", I mean it happens to match the loop you care about, but doesn't check that the rotated loop will simplify.  Even if you find a PHI in the header which matches your pattern, rotating the loop might not remove it.
Ok. I see. It should be checking that the exit lcssa phi is from the header, not other exits. Otherwise the phi will not be simplified. I'll update things.


https://reviews.llvm.org/D44199