[PATCH] D105216: [ScalarEvolution] Fix overflow in computeBECount.

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

I'm working on a very similar patch as we speak, so you found a knowledgeable reviewer.  Your test diff is a lot smaller than my current one, so maybe you're on to something.  Unfortunately, I'm about to leave for the day, so I won't be able to get you detailed feedback until at least tomorrow.

A couple of quick comments.

The code does disallow stride == 0 on the howManyLessThans path.  The logic is explained in the !PositiveStride path, but the comments are really confusing, so let me explain again.  For a non-positive exit, we require that this be the sole exit and the current condition control that exit.  (Indirectly through NoWrap)  As a result, if the stride was zero, then the loop must either a) be infinite, or b) exit on the first iteration.  The !loopIsFiniteByAssumption check requires a zero stride to exit on the first iteration.  (Ah, I think I see the issue you did now.  The stride can be zero iff the exit is taken on first iteration.  If this was your chain of thought, can you clarify submission comment?)



================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:11532
+  const SCEV *Delta = getMinusSCEV(End, Start);
+  if (!MayOverflow) {
+    // floor((D + (S - 1)) / S)
----------------
Style comment: I'd suggest pulling out a function getUDivCeiling(Delta,Stride,MayOverflow).  I found (in my WIP patch) that this really helps readability.  Once you have that, you can use early return via tail call to helper for most of the above.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:11541
+  //
+  // FIXME: We can skip the umin if we prove the backedge is taken at least
+  // once.
----------------
While true, this is the same set of facts which allowed you to avoid the max for the start expression. As such, you've already gotten most of the benefit.  


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:11643
   const SCEV *BECount;
   if (isLoopEntryGuardedByCond(L, Cond, getMinusSCEV(OrigStart, Stride), OrigRHS))
     BECount = BECountIfBackedgeTaken;
----------------
This check needs removed.  The problem is that this isn't doing what the comment says it is.

The actual condition to prove the backedge is taken on the first iteration is OrigStart Cond OrigRHS.  

I have a test case which this check causes us to generate wrong code because Start-Stride < RHS, but Start < RHS does not hold.  (Actually, what happened to that test case?  I thought I'd committed that, but I don't see it in my commit history.  Need to find that.)

When I tried removing this without also fixing the overflow issue, I got bad results.  I don't fully remember why atm, will try to reconstruct that and share it.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105216