[PATCH] D102635: [LoopUnroll] Use tripcount from exiting header, if latch not exiting.

[Florian Hahn via Phabricator via llvm-commits]

fhahn updated this revision to Diff 346001.
fhahn added a comment.

In D102635#2764588 <https://reviews.llvm.org/D102635#2764588>, @Meinersbur wrote:

> In D102635#2764456 <https://reviews.llvm.org/D102635#2764456>, @reames wrote:
>
>> This is semantically incorrect, and can not land.  There is an important distinction between a maximum trip count (which this code would be correct for), and an exact trip count (which is what this code appears to need).  Consider the case where the header exits after 20 iterations, and another non-latch exit exits after 10.  Fully unrolling by 20 would be incorrect.  (Since control flow is elided and we'd execute too many iterations.)
>
> Does this mean only the branch in the latch is elided by (full) unrolling? The situation could also be that the latch exits after 20 iterations. I was not expecting that there is something special about the latch when exiting.

Thanks for taking a look! LoopUnroll expects TripCount to be an upper bound on the number of times the terminator we simplify during unrolling executes. At the moment, that's either the latch block terminator, if it exits or the unique exiting block's termaintor. Otherwise the exiting branches should be retained. I've also added another test that has an early exit depending on the IV as well. It gets simplified, but only due to simplifications after unrolling.

If there's a single exit block or the latch is exiting, we use TripCount to decide whether the loop is fully unrolled. If that's the case, we simplify the branches in the latch/unique exiting block. If there the latch is not exiting and there's no unique exiting block, we won't simplify the exiting branches to unconditional branches (the code to do so is around https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/Utils/LoopUnroll.cpp#L787, but ExitingBlocks will be empty, if there's no single exiting branch instruction).

I also had another look at some of the comments and it seems like the one for ` llvm::UnrollLoop` was not updated to reflect the fact that loops with multiple exiting blocks are also supported. I tried to update the comment to reflect the current requirement. I might have missed something (there' been a few other changes since I added support for unrolling header-exiting loops and quite a few cases are handled in the code), but I think it should more accurately reflect what's going on. I think it might make sense to split off the comment change, but I included it here to start with as it is directly related to the discussion here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102635

Files:
  llvm/lib/Transforms/Scalar/LoopUnrollPass.cpp
  llvm/lib/Transforms/Utils/LoopUnroll.cpp
  llvm/test/Transforms/LoopUnroll/unroll-header-exiting-with-phis-multiple-exiting-blocks.ll

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D102635.346001.patch
Type: text/x-patch
Size: 10817 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210517/e1b8fbbd/attachment.bin>