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

[Nikita Popov via Phabricator via llvm-commits]

nikic accepted this revision.
nikic added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: llvm/lib/Transforms/Utils/LoopUnroll.cpp:415
+  const unsigned ExactTripCount = ExitingBI ?
+    SE->getSmallConstantTripCount(L,ExitingBI->getParent()) : 0;
+  const bool ExactUnroll = (ExactTripCount && ExactTripCount == ULO.Count);
----------------
nit: Space after comma.


================
Comment at: llvm/lib/Transforms/Utils/LoopUnroll.cpp:760
       if (CompletelyUnroll) {
-        if (ULO.PreserveCondBr && j && !(PreserveOnlyFirst && i != 0))
-          return None;
-        return j == 0;
+        ;
+        if (PreserveOnlyFirst) {
----------------
nit: Stray semicolon.


================
Comment at: llvm/lib/Transforms/Utils/LoopUnroll.cpp:770
+        if (j == 0)
+          return true;
+        if (MaxTripCount && j >= MaxTripCount)
----------------
It may make sense to move the `if (j == 0) return false;` case to the start, as it's always the same. With that done, you should be able to drop the separate check for `ExactUnroll`, as the `ExactTripCount` case below should cover it.


================
Comment at: llvm/lib/Transforms/Utils/LoopUnroll.cpp:770
+        if (MaxTripCount && j >= MaxTripCount)
+          return false;
+        if (ExactTripCount && j != ExactTripCount)
----------------
reames wrote:
> nikic wrote:
> > 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`.
> I'm not quite sure what you're asking for.  It sounds like maybe you want me to change how many iterations we unroll when ULO.Count > MaxTripCount?
> 
> If so, I definitely request that be a separate patch.  I'm trying to avoid large test diffs with each incremental patch and changing too much at once makes the test diffs really hard to understand.
Fair enough, I'm happy to have that in a followup. I mainly suggested it here because we would not have to worry about `j >= MaxTripCount` situations.


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

https://reviews.llvm.org/D103620