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

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev requested changes to this revision.
mkazantsev added inline comments.
This revision now requires changes to proceed.


================
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.
----------------
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.


================
Comment at: lib/Transforms/Utils/LoopUnrollPeel.cpp:184
+    } else if (isa<SCEVAddRecExpr>(RightSCEV))
+      continue;
+
----------------
After all this logic, could you please create a variable like `LeftAR = cast<SCEVAddRecExpr>(LeftSCEV)` to make it explicitly clear what you expect to see next? I also suggest bailing out if `LeftAR->isAffine()` is `false` because it will lead you to huge SCEV computations in the loop below.


================
Comment at: lib/Transforms/Utils/LoopUnrollPeel.cpp:187
+    // Check if extending DesiredPeelCount lets us evaluate Pred.
+    APInt C(16, DesiredPeelCount);
+    const SCEV *IterVal = cast<SCEVAddRecExpr>(LeftSCEV)->evaluateAtIteration(
----------------
Any good reason why it is 16 bit?


================
Comment at: lib/Transforms/Utils/LoopUnrollPeel.cpp:189
+    const SCEV *IterVal = cast<SCEVAddRecExpr>(LeftSCEV)->evaluateAtIteration(
+        SE.getConstant(C), SE);
+    if (!SE.isKnownPredicate(Pred, IterVal, RightSCEV))
----------------
You could have used `SE.getConstant(LeftSCEV->getType(), DesiredPeelCount)` and no need to declare `C` for this.


================
Comment at: lib/Transforms/Utils/LoopUnrollPeel.cpp:190
+        SE.getConstant(C), SE);
+    if (!SE.isKnownPredicate(Pred, IterVal, RightSCEV))
+      Pred = ICmpInst::getSwappedPredicate(Pred);
----------------
I totally don't get this logic. Are we going to prove _another_ predicate if SCEV was unable to prove something for this one? Could you please add a comment on what is going on here?


================
Comment at: lib/Transforms/Utils/LoopUnrollPeel.cpp:194
+           SE.isKnownPredicate(Pred, IterVal, RightSCEV)) {
+      IterVal = SE.getAddExpr(
+          IterVal, cast<SCEVAddRecExpr>(LeftSCEV)->getStepRecurrence(SE));
----------------
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.


https://reviews.llvm.org/D43876