[PATCH] D34532: [LoopUnroll] Fix bug in computeUnrollCount causing it to not honor MaxCount

[Geoff Berry via Phabricator via llvm-commits]

gberry added inline comments.


================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:839
     }
+    if (UP.Count > UP.MaxCount)
+      UP.Count = UP.MaxCount;
----------------
anna wrote:
> What happens if we early return from this function with result as `true`, but UP.Count > UP.MaxCount?
> 
> Perhaps we should be setting the max value at the caller of this function :
> ```
> tryToUnrollLoop() {
> ....
>  // computeUnrollCount() decides whether it is beneficial to use upper bound to fully 
>  // unroll the loop.
>   bool UseUpperBound = false;
>   bool IsCountSetExplicitly =
>       computeUnrollCount(L, TTI, DT, LI, SE, &ORE, TripCount, MaxTripCount,
>                          TripMultiple, LoopSize, UP, UseUpperBound);
>   if (!UP.Count)
>     return false;
>   // Unroll factor (Count) must be less or equal to TripCount.
>   if (TripCount && UP.Count > TripCount)
>     UP.Count = TripCount;
>   // Add the check after this?
> 
> }
> ```
I think this is the only place that MaxCount wasn't being honored where it should be.  The earlier checks that return early are:
- explicit -unroll-count settings and pragma unroll counts, which I don't think should be affected by MaxCount
- full unroll count, which has a separate max: FullUnrollMaxCount which seems to be honored
- loop peeling: sets Count to 1, so MaxCount should not have any effect


https://reviews.llvm.org/D34532