[PATCH] D28393: [SCEV] Make howFarToZero produce a smaller max backedge-taken count

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy requested changes to this revision.
sanjoy added a comment.
This revision now requires changes to proceed.

I'm happy with the direction, but I think this is changing the code in two ways at once.  I'd rather have the two changes clearly spelt out as different checkins.



================
Comment at: lib/Analysis/ScalarEvolution.cpp:7216
+    const SCEVConstant *One = cast<SCEVConstant>(getOne(Distance->getType()));
+    const SCEV *DistancePlusOne = getAddExpr(Distance, getOne(Distance->getType()));
+    if (isLoopEntryGuardedByCond(L, ICmpInst::ICMP_NE, DistancePlusOne, Zero)) {
----------------
s/`getAddExpr(Distance, getOne(Distance->getType()))`/`getAddExpr(Distance, One)`/ ?


================
Comment at: lib/Analysis/ScalarEvolution.cpp:7219
+      ConstantRange CR = getUnsignedRange(DistancePlusOne);
+      APInt NewMaxBECount = CR.getUnsignedMax() - One->getAPInt();
+      if (NewMaxBECount.ult(MaxBECount))
----------------
Why are you checking `(Distance + 1) != 0` instead of `Distance != -1`?  I'd find the latter more straightforward.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:7210
+    APInt MaxBECount = getUnsignedRange(Distance).getUnsignedMax();
+
+    // When a loop like "for (int i = 0; i != n; ++i) { /* body */ }" is rotated,
----------------
I think we should really split this change up into two.

 - First the cleanup to handle count-up loops and count-down loops more uniformly (i.e. first reduce to `{D,+,-1}` and then compute `getUnsignedMax(D)`) than the current ad-hoc logic.  IIUC, this change is already an improvement for cases like `{X,+,1} != 0` if range(`X`) == `[-5, 1)` (say).
 - Add the smartness around `unsigned_max(D) == unsigned_max(D + 1) - 1` on no overflow in a separate commit.


https://reviews.llvm.org/D28393