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

[Eli Friedman via Phabricator via llvm-commits]

efriedma 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)));
----------------
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.


https://reviews.llvm.org/D44199