[PATCH] D102982: [LoopUnroll] Use smallest exact trip count from any exit

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopUnrollPass.cpp:1119
+  L->getExitingBlocks(ExitingBlocks);
+  for (BasicBlock *BB : ExitingBlocks) {
+    // Only unroll against latch-dominating exit blocks.
----------------
reames wrote:
> nikic wrote:
> > reames wrote:
> > > I would strongly prefer this logic be sunk into the appropriate SCEV accessors.  I'm fine with you doing that in a follow up provided you commit to doing so.  I'll leave that decision up to you.  
> > Could you please clarify which SCEV accessors you have in mind here? Do you mean sinking this into the getSmallConstantTripCount() variant that only accepts a Loop? I would have a couple of concerns with doing that:
> > 
> >  * We also need to know which exit the trip count refers to.
> >  * This is not really the trip count of the loop (just a loop exit), and I'm pretty sure changing that would break other users of the API.
> >  * The limitation to latch-dominating exits here is not fundamental, and mainly there due to unclear profitability.
> > 
> > Maybe I misunderstood the suggestion though.
> I was referring specifically to the small constant trip count and small constant multiple versions which take Loop parameters.  
> 
> Your point about needing the exit block is true for the way the code is currently phrased.  I'd missed that.  I think the need for that can and should be removed (see my comment on the patch this one is based on), but if that's logistically complicated, I'm fine with us moving forward with this structure and then revisiting in the future.  
> 
> Any exit count for an exit which dominates the latch must be a (potentially conservative) exit count for the loop.  So, I'm not quite sure what you mean with the rest of your comments.  
> Your point about needing the exit block is true for the way the code is currently phrased. I'd missed that. I think the need for that can and should be removed (see my comment on the patch this one is based on), but if that's logistically complicated, I'm fine with us moving forward with this structure and then revisiting in the future.

Going to respond here to keep it in one place: I think what you're suggesting is effectively to use getSmallConstantMaxTripCount() and pass down that information only. However, the important distinction here is that the max trip count only tells you that branches after that trip count must be taken, not that branches before it cannot be taken (for that specific exit). Unrolling handles a number of different exits (exact, exact-or-zero, max, multiple-of) and this (unless I'm misunderstanding again) would only handle the max case, where we can only fold the final branch.

> Any exit count for an exit which dominates the latch must be a (potentially conservative) exit count for the loop. So, I'm not quite sure what you mean with the rest of your comments.

The getSmallConstantTripCount() currently returns an exact trip count for a single-exit loop, while getSmallConstantMaxTripCount() returns a max trip count for any loop. As mentioned before, both provide different guarantees. I think to stick with the spirit of getSmallConstantTripCount(), it should only be returning a value if all the exit counts in a loop match, which doesn't seem terribly useful in practice.


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

https://reviews.llvm.org/D102982