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

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy accepted this revision.
sanjoy added a comment.

lgtm with minor comments



================
Comment at: lib/Transforms/Utils/LoopUnrollPeel.cpp:94
+
+  BasicBlock *BackEdge = L->getLoopLatch();
+  assert(BackEdge && "Loop is not in simplified form?");
----------------
sanjoy wrote:
> This is a suggestion, feel free to ignore:
> 
> `getLoopLatch` isn't super-cheap (it re-discovers the latch every time).  How about passing the latch explicitly to this function?
Please add an assert that `BackEdge` is, in fact, the latch.


================
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"
----------------
mkazantsev wrote:
> mkazantsev wrote:
> > 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.
> Line 131 rejects empty loops:
>   if (!L->empty())
>     return;
> 
I don't think line 131 has anything to do with `LoopSize`, it is just checking if there are *subloops* or not.

But I think your point on `LoopSize` being non-zero is correct.  Can you please add an assert?


================
Comment at: lib/Transforms/Utils/LoopUnrollPeel.cpp:168
+                   << " of some Phis forming an invariant chain.\n");
+      UP.PeelCount = DesiredPeelCount;
       return;
----------------
mkazantsev wrote:
> 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.
SGTM.


https://reviews.llvm.org/D31613