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

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy added a comment.

Now that the code is in APInt we should have some tests in `unittests/ADT/APIntTest.cpp`.  Can we pull in some subset of https://reviews.llvm.org/D50095 that runs within a reasonable amount of time (only bitwidth 4, for instance)?

I also have some "black box" comments inline on how the function is documented.



================
Comment at: include/llvm/ADT/APInt.h:2174
+/// (a) n >= 0 and q(n) = 0, or
+/// (b) n >= 1 and q(n-1) and q(n) belong to two different intervals
+///     [Rk, Rk+R), where R = 2^BW, and k is an integer.
----------------
Would be nice to clarify the bitwidth in which you evaluate `q(n)` for (b).  It can't be `BW` since any value in [0,2^BW) will either lies in exactly one such interval or all such intervals, depending on how you slice it.


================
Comment at: include/llvm/ADT/APInt.h:2178
+/// same time "allowing" subtraction. In unsigned modulo arithmetic a
+/// subtraction would always count as an overflow, but here we want to
+/// allow values to decrease and increase as long as they are within
----------------
I'm not sure this is correct -- `sub 5, 1` does not unsign-overflow right?  (since zext(5) - zext(1) == zext(5 - 1))?


================
Comment at: include/llvm/ADT/APInt.h:2183
+/// This function returns None when
+/// (a) either of the coefficients are not constant, or
+/// (b) after finding k that minimizes the positive solution to q(n) = kR,
----------------
Is (a) relevant any more?  All the coeffs are APInt.


================
Comment at: include/llvm/ADT/APInt.h:2187
+///
+/// There are cases where q(n) > T, and q(n+1) < T by the virtue of *signed*
+/// overflow. This function will *not* find such an n, however it may find
----------------
Can you add a line on how this "q(n) > T, and q(n+1) < T" ties in with the return value of this function specified at the very top "n >= 1 and q(n-1) and q(n) belong to two different intervals..."?


Repository:
  rL LLVM

https://reviews.llvm.org/D48283