[PATCH] D73058: [LoopRotate] add ability to repeat loop rotation until non-deoptimizing exit is found

[Serguei Katkov via Phabricator via llvm-commits]

skatkov accepted this revision.
skatkov added a comment.
This revision is now accepted and ready to land.

Please wait for one or two days before landing for comments from others.



================
Comment at: llvm/lib/Transforms/Utils/LoopRotationUtils.cpp:232
+    return any_of(Exits, [](const BasicBlock *BB) {
+      return !BB->getPostdominatingDeoptimizeCall();
+    });
----------------
fedor.sergeev wrote:
> skatkov wrote:
> > Side comment, May be it makes sense to keep note here in comment.
> > As I understand getPostdominatingDeoptimizeCall may return false even if it ends with deopt. Say if you have a loop before deoptimization. In this case there is no need to rotate but we will do.
> Yep, situation when all exits are in fact postdominated by calls to deoptimize is possible.
> However it is a really degenerate case (every exit is unlikely, so the loop is either unlikely itself or it is an endless loop, thus does not have finite trip-count).
> We should not care what we do here.
My point was just to put a comment here that getPostdominatingDeoptimizeCall  may return false conservatively.



================
Comment at: llvm/lib/Transforms/Utils/LoopRotationUtils.cpp:570
+    // to handle multiple rotations in one go.
+    rotateLoop(L, false);
+  }
----------------
fedor.sergeev wrote:
> skatkov wrote:
> > Does'not you want to convert this recursion into iteration by moving this code to LoopRotate::processLoop?
> Then it makes the patch looking much bigger than it actually is due to lots of whitespace changes.
> I'm not sure it is worth the effort.
> Perhaps do that separately?
> The only benefit is that in loop form it becomes super-easy to limit amount of iterations.
> 
Ok, but still I like iterations more than recursions here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73058