[PATCH] D43876: [LoopUnroll] Peel off iterations if it makes conditions true/false.

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added inline comments.


================
Comment at: lib/Transforms/Utils/LoopUnrollPeel.cpp:173
     }
+
+    // Pay respect to limitations implied by loop size and the max peel count.
+    unsigned MaxPeelCount = UnrollPeelMaxCount;
+    MaxPeelCount = std::min(MaxPeelCount, UP.Threshold / LoopSize - 1);
+
+    DesiredPeelCount = std::max(DesiredPeelCount,
+                                countToEliminateCompares(*L, MaxPeelCount, SE));
+
     if (DesiredPeelCount > 0) {
-      // Pay respect to limitations implied by loop size and the max peel count.
----------------
fhahn wrote:
> mkazantsev wrote:
> > What was the point of moving this `if`? Could we not just update DesiredPeelCount before this line?
> > We only need MaxPeelCount under this condition, there is no point in calculating it before it.
> MaxPeelCount is passed to `countToEliminateCompares`, to limit the maximum iterations we do.
My bad, I didn't notice it. :)


================
Comment at: lib/Transforms/Utils/LoopUnrollPeel.cpp:194
+           SE.isKnownPredicate(Pred, IterVal, RightSCEV)) {
+      IterVal = SE.getAddExpr(
+          IterVal, cast<SCEVAddRecExpr>(LeftSCEV)->getStepRecurrence(SE));
----------------
fhahn wrote:
> mkazantsev wrote:
> > Step calculation can be hoisted out of this loop. I would also suggest bailing early if AR is not affine because adding a step of non-affine AddRec many times can produce really big and ugly SCEVs.
> I've added a comment that makes it clearer I hope. The idea is to handle cases like below, where the condition is known to be false initially. Initially `i > 2` is not known, but the inverse `i  <= 2` is known.
> 
> ```
> if (i > 2) {
>   // do something
> } else {
>   // do something else
> }
> ```
I see the point now, but you are using wrong function. If you look into `getSwappedPredicate`, it returns `<` for `>`, and what you need is called `getInversePredicate`.


https://reviews.llvm.org/D43876