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

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

Would be LGTM, but I'd like to understand the called out test change first.  Everything else looks as expected.



================
Comment at: llvm/lib/Transforms/Scalar/LoopUnrollPass.cpp:1124
+  // Find the smallest exact trip count for any exit. An unroll by this trip
+  // count will eliminate at least one exit, but there may be additional exits
+  // beyond it.
----------------
Your comment here is wrong.  The code is correct, but the comment isn't.  :)

If we unroll by the exact trip count of *any* exit, we're guaranteed to break the backedge.  As such, there might be conditional exits left in *earlier* iterations, but there will be nothing in *later* iterations which is what your comment appears to say.  

It might also be worth stating explicitly that this is an upper bound on the actual trip count of the loop (since an earlier conditional exit we can't analyze might be taken), and draw the distinction with a maximum count (conservatism in analyzing each exit.)  Separately, I really think we should be allowing max trip counts here, but that's a separate step.  


================
Comment at: llvm/test/Transforms/LoopUnroll/runtime-loop-known-exit.ll:9
 
 define void @test(i32 %s, i32 %n) {
 ; CHECK-LABEL: @test(
----------------
This test change looks concerning.  Have you explored why this is happening?  On the surface, this looks like a bad interaction between runtime unrolling and finding a more precise trip count for full unrolling we may need to explore.


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

https://reviews.llvm.org/D102982