[PATCH] D48283: [SCEV] Properly solve quadratic equations

[Krzysztof Parzyszek via Phabricator via llvm-commits]

kparzysz added inline comments.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:10710
+      return UpperValid ? SE.getConstant(SU.getValue())
+                        : SE.getConstant(SL.getValue());
+
----------------
efriedma wrote:
> kparzysz wrote:
> > kparzysz wrote:
> > > efriedma wrote:
> > > > I'm not convinced this is correct: why is the "valid" solution guaranteed to be smaller?
> > > Are you referring to line 10720?  Because it's the number of iterations. Both solutions are positive and both are validated as leaving the range, so the smaller one will happen first.
> > In this line, either UpperValid or LowerValid are true, but not both.  If UpperValid is false, then the lower solution is.
> I'm specifically concerned about the case where `U.ult(L)` is true, and UpperValid is false (so we return a "valid" solution, but not the smallest solution).  Or is that impossible for some reason?
Yeah, there is a problem.

We need to handle signed overflow as well.  The validity of the solution checked here only checks that it leaves the range, but it doesn't check that it's the first one to do so.  The solving functions needs to give the guarantee of being the smallest solution.


Repository:
  rL LLVM

https://reviews.llvm.org/D48283