[PATCH] D64972: [Loop Peeling] Do not close further unroll/peel if not full peeling happens

Mon Jul 22 02:02:37 PDT 2019

skatkov marked an inline comment as done.
skatkov added a comment.

In D64972#1593826 <https://reviews.llvm.org/D64972#1593826>, @reames wrote:

> I'm not quite sure this is the right macro approach.
>
> The comment near the code your modifying makes me thing the whole reasoning behind disabling future peeling and unrolling after the first peel may be flawed.  It doesn't make sense to "use up" profiling information.  Assuming we correctly updated our profiling when doing the transform, the resulting profile for the loop should indicate that it's cold and thus not profitable to further peel/unroll.  If that's not happening, maybe there's another issue in play?  (I wonder if your other profile bug fix may help here?)
>
> p.s. I'm opened to being convinced that this is a practical answer, even if not an ideal one.  Just make the argument.  :)

Hi Philip, the story is the following.

Let's we have a loop which does 5 iteration according to profile but also one of its condition can be simplified by peeling and to simplify condition we need only 1 iteration peeled off.
According to current implementation Loop Peeling cost model will decide that we should peel off one iteration.
One iteration is peeled off and weights are updated correctly (now estimated trip count is 5 - 1 == 4)
But we mark the loop as llvm.loop.unroll.disable unconditionally if we do any peeling.

As a result another potential LoopUnroll pass will not even consider this loop for peeling while if it does it would detect that we can peel additional 4 iterations.

So this patch fixes this part: if we did not peel all iteration basing on profile we should mark this loop as no consider for future peel/unroll.

Will update the patch soon with modified proposed name of variable which really makes the intention clearer.

================
Comment at: llvm/include/llvm/Analysis/TargetTransformInfo.h:478
     bool UnrollAndJam;
+    /// Allow full peeling. Uses to enable peeling off all iterations which
+    /// closes further unroll/peeling optimizations on the loop.
----------------
reames wrote:
> Naming wise: Please call this something other than "Full" peeling.  Full peeling sounds like full unrolling which would imply we were able to entirely eliminate the loop structure.
> 
> Maybe: PeelProfiledIterations?
Agreed.

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

https://reviews.llvm.org/D64972