[PATCH] D31613: [LoopPeeling] Get rid of Phis that become invariant after N steps

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added inline comments.


================
Comment at: lib/Transforms/Utils/LoopUnrollPeel.cpp:165
+      DesiredPeelCount = std::min(DesiredPeelCount, MaxPeelCount);
+      assert(DesiredPeelCount > 0 && "Wrong loop size estimation?");
+      DEBUG(dbgs() << "Peel " << DesiredPeelCount << " iteration(s) to get rid"
----------------
sanjoy wrote:
> Can `UP.Threshold` be `0` (say it was directly set by the user by `UnrollThreshold`)?
Only if loop size is also 0, we enter here witn condition 2 * LoopSize <= UP.Threshold :) So I Don't think it's real.


================
Comment at: lib/Transforms/Utils/LoopUnrollPeel.cpp:168
+                   << " of some Phis forming an invariant chain.\n");
+      UP.PeelCount = DesiredPeelCount;
       return;
----------------
sanjoy wrote:
> I don't know the rest of the code well enough to be conclusive on this, but should this be `UP.PeelCount = std::max(UP.PeelCount, DesiredPeelCount);`?  That is, if we've earlier decided, due to some other reasons, that peeling for 5 iterations is a good idea, and here we decide that peeling for only 3 iterations is a good idea, perhaps we should still peel for 5 iterations?
> 
> Actually, even if `UP.PeelCount` > `DesiredPeelCount` is impossible, I think `UP.PeelCount = std::max(UP.PeelCount, DesiredPeelCount);` seems cleaner.
Line 126

   UP.PeelCount = 0;

It is never changed after that. The puspose of this method is to only set it once.


https://reviews.llvm.org/D31613