[PATCH] D63923: [Loop Peeling] Enable peeling for loops with multiple exits

[Serguei Katkov via Phabricator via llvm-commits]

skatkov marked 2 inline comments as done.
skatkov added inline comments.


================
Comment at: lib/Transforms/Utils/LoopUnrollPeel.cpp:65
+static cl::opt<bool>
+UnrollPeelMultiExit("unroll-peel-multi-exit", cl::init(true), cl::Hidden,
+                    cl::desc("Allow peeling of loops with multiple exits."));
----------------
reames wrote:
> Submit disabled, then enable in future commit?
> 
> And maybe you want a three way choice: none, deopt-only, all?
Sure, will change to false.

I do not support the non deopt-only now in terms of update of branch weights. So introducing the explicit difference between deopt-only and all is not what I think make sense.


================
Comment at: lib/Transforms/Utils/LoopUtils.cpp:626
   if (!L->getExitingBlock())
     return None;
+  return getLoopEstimatedTripCountBasingOnLatchOnly(L);
----------------
reames wrote:
> A suggestion here.  Extend the existing function to allow multiple exits, but only if those exits have 0 taken probability.  Return None for any exit with non-zero exit.  
> 
> (I'm trying to avoid the need for the new function.)
Unfortunately, zero probability is not what is used in llvm. Even BranchProbability analysis will set smallest but greater than zero probability for edge coming to block with unreachable terminator.

So if want to add a complexity to this method we should introduce some threshold below which we consider the exit has '0' probability.

Do you still see the value in it?
Please note that getLoopEstimatedTripCount is widely used in code base.



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

https://reviews.llvm.org/D63923