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

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

In D105216#2851010 <https://reviews.llvm.org/D105216#2851010>, @reames wrote:

> Your test diff is a lot smaller than my current one, so maybe you're on to something.

I found that I needed a wide variety of heuristics to handle various cases in the regression tests.  This is the source of most of the complexity in this patch.

> (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?)

I'll clarify the commit message, sure.



================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:11532
+  const SCEV *Delta = getMinusSCEV(End, Start);
+  if (!MayOverflow) {
+    // floor((D + (S - 1)) / S)
----------------
reames wrote:
> 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.
Hmm...

Maybe I'll just make the computation of MayOverflow a lambda.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:11643
   const SCEV *BECount;
   if (isLoopEntryGuardedByCond(L, Cond, getMinusSCEV(OrigStart, Stride), OrigRHS))
     BECount = BECountIfBackedgeTaken;
----------------
reames wrote:
> 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.  
Okay.  We can deal with this separately, I think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105216