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

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/LoopUnroll.cpp:770
+        if (j == 0)
+          return true;
+        if (MaxTripCount && j >= MaxTripCount)
----------------
nikic wrote:
> 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.
Landed as is, then did this in cddcc4cf.  This code is complicated enough that I want something easy to revert if this final style change turns out be buggy.  :)


================
Comment at: llvm/lib/Transforms/Utils/LoopUnroll.cpp:770
+        if (MaxTripCount && j >= MaxTripCount)
+          return false;
+        if (ExactTripCount && j != ExactTripCount)
----------------
nikic wrote:
> 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.
I'm definitely going to hold on doing that until after we split out peeling.  I don't believe the current logic is even correct when there's a non-zero peel count.  I don't want to build on it yet.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103620