[PATCH] D104487: [LoopUnroll] Push runtime unrolling decision up into tryToUnrollLoop()

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/LoopUnroll.cpp:391
+        assert((!HasConvergent || !ULO.Runtime) &&
+               "Can't runtime unroll if loop contains a convergent operation.");
       });
----------------
I'm not particularly familiar with convergent operations, but looking back at D17526 the concern here seems to have been specifically about the prologue introduced by runtime unrolling, not anything else.


================
Comment at: llvm/lib/Transforms/Utils/LoopUnrollAndJam.cpp:251
   // We use the runtime remainder in cases where we don't know trip multiple
-  if (TripMultiple == 1 || TripMultiple % Count != 0) {
+  if (TripMultiple % Count != 0) {
     if (!UnrollRuntimeLoopRemainder(L, Count, /*AllowExpensiveTripCount*/ false,
----------------
This change deserves a comment: If TripMultiple is 1 and Count is 1 and UnrollRemainder was set, this tried to unroll the remainder loop with a Count of 0, which is not legal. It wasn't caught previously due to the check
```
// Don't enter the unroll code if there is nothing to do.
if (ULO.TripCount == 0 && ULO.Count < 2) {
```
which happened before the `assert(Count > 0)` assertion. I've removed that check and the assertion is triggered now.

The change here adjust things to not try to create a remainder loop for an unroll count of 1, as this doesn't make sense in the first place.


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

https://reviews.llvm.org/D104487