[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:380
+  const unsigned ExactTripCount = SE->getSmallConstantTripCount(L);
+  const bool ExactUnroll = (ExactTripCount && ExactTripCount == ULO.Count);
 
----------------
nikic wrote:
> 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.
Good suggestion, incorporated.

JFYI, ExitingBI is on my list to kill, but not in this patch.  :)


================
Comment at: llvm/lib/Transforms/Utils/LoopUnroll.cpp:770
+        if (MaxTripCount && j >= MaxTripCount)
+          return false;
+        if (ExactTripCount && j != ExactTripCount)
----------------
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.


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

https://reviews.llvm.org/D103620