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

[Krzysztof Parzyszek via Phabricator via llvm-commits]

kparzysz marked 4 inline comments as done.
kparzysz added inline comments.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:8473
+    LLVM_DEBUG(dbgs() << __func__ << ": no valid solution\n");
+    return None;
+  }
----------------
efriedma wrote:
> Do we have to return None here?  Even if we can't return a root, we should still be able to find the point where it wraps.
> 
> If you think None is the correct representation, please ensure SolveQuadraticAddRecRange doesn't succeed in that case.
Yes, this case could likely be solved, but we fail to do it.

The code above that updates the C in Ax^2+Bx+C, does that in a way to find the smallest x that causes an overflow (or zero). It uses the standard "real number" analysis for that, which only assures the existence of such a solution (as a real number), but does nothing to make sure that there will be an integer that satisfies the requirements.  If a single iteration skips over both zeros of the updated equation then both, the previous and the next iteration will produce positive values and this case will go "unnoticed" by the loop. The code updating C could, in theory, do that to make sure that an integer solution exists, I just haven't thought of an elegant way to do it.

The point about SolveQuadraticAddRecRange is definitely valid and I have fixed it.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:8558
+/// solution fits in the original bit width, truncate it (except for i1).
+/// Returning a value of a different bit width may inhibit some optimizations.
+static Optional<APInt> TruncIfPossible(Optional<APInt> X, unsigned BitWidth) {
----------------
efriedma wrote:
> Is it possible for the truncation to fail?  For an AddRec of bitwidth N, evaluateAtIteration(2^N)==evaluateAtIteration(0), so any solution must be less than 2^N.  (And all users should treat the backedge-taken count as an unsigned quantity.)
It is possible.  The test `test/Analysis/ScalarEvolution/solve-quadratic-i1.ll` has coefficients in i1, but the solution is 2 (the loop iterates 3 times).  I'm not sure if this is specific to i1 or not.  It hasn't happened in any test I've run outside of i1.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:8657
+    // Return the value that exists, if both exist pick the smaller one.
+    return LessOptional(SO, UO);
+  };
----------------
efriedma wrote:
> Do we need to check which one is smaller before we check LeavesRange?
We don't need to, but we can.


Repository:
  rL LLVM

https://reviews.llvm.org/D48283