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

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

I like the separate entry points for equality and inequality.

I was curious about exact solutions, so I did a bit of searching; https://arxiv.org/pdf/1711.03621.pdf describes the complete solution for equality.



================
Comment at: lib/Analysis/ScalarEvolution.cpp:8305
+    B.negate();
+    C.negate();
+  }
----------------
negate() can overflow; if that overflow is impossible because of the way the inputs are constructed, please add an assertion.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:8473
+    LLVM_DEBUG(dbgs() << __func__ << ": no valid solution\n");
+    return None;
+  }
----------------
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.


================
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) {
----------------
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.)


================
Comment at: lib/Analysis/ScalarEvolution.cpp:8579
+/// (a) the addrec coefficients are not constant, or
+/// (b) SolveQuadraticEquation was unable to find a solution.
+static Optional<APInt>
----------------
You might want to explicitly state that there is no solution in some cases.


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


Repository:
  rL LLVM

https://reviews.llvm.org/D48283