[PATCH] D103620: [LoopUnroll] Eliminate PreserveCondBr parameter and fix a bug in the process

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/LoopUnroll.cpp:380
+  const unsigned ExactTripCount = SE->getSmallConstantTripCount(L);
+  const bool ExactUnroll = (ExactTripCount && ExactTripCount == ULO.Count);
 
----------------
I don't like this. This not resulting in regressions just means we have bad test coverage. I've added an extra test in https://reviews.llvm.org/rG33e41eaecdd7 that should fail after this change.

It would be better to base this on the exact trip count of `ExitingBlock` (below) and then generalize from there.


================
Comment at: llvm/lib/Transforms/Utils/LoopUnroll.cpp:770
+        if (MaxTripCount && j >= MaxTripCount)
+          return false;
+        if (ExactTripCount && j != ExactTripCount)
----------------
I think it would be better to clamp ULO.Count to MaxTripCount upfront, and avoid these special cases for MaxTripCount and ExactTripCount. It both makes the code simpler, and the unrolling result simpler.

This is actually already done at `Effectively "DCE" unrolled iterations` above, but we should do it based on the MaxTripCount, not `ULO.TripCount`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103620