[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: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.
----------------
nikic wrote:
> reames wrote:
> > 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.  
> What I really wanted to say here is that an unroll by this trip count eliminates all branches relating to one exit, but branches relating to other exits may have to be kept. This is opposed to the max trip count case where we're only guaranteed to break the backedge, but may not be able to remove any other branches.
> 
> The unroll code already handles max trip count, but it's only used if no exact trip count is known, and is controlled by a target option (which is disabled on X86...)
I guess it's worth discussing the larger context here. It probably doesn't come as a surprise that the modelling is rather odd and doesn't seem particularly principled. https://github.com/llvm/llvm-project/blob/59d90fe817b5f1feae1a1406bd487e6552b9928d/llvm/lib/Transforms/Scalar/LoopUnrollPass.cpp#L835-L846 lays out the reason why this is behind a target option, which is that it will result in more branches, which may be problematic for constrained branch predicators. What this doesn't take into account is that even a full unroll may replicate branches, either for other exits or just control flow within the loop.

What would make more sense to me is to have some kind of "branch penalty" that applies for each newly introduced branch -- this could be due to inner control flow, a remaining unpredictable exit, or an exit that only has an upper bound.


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

https://reviews.llvm.org/D102982