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

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

Needs more test coverage of the overflow cases.



================
Comment at: lib/Analysis/ScalarEvolution.cpp:8269
+  uint32_t NewWidth = std::max(32u, 2*BitWidth);
+  if (BitWidth == 1) {
+    N = N.zext(NewWidth);
----------------
I don't like hardcoded numbers without an explanation... why is 32 special?  Why is 1 special?


================
Comment at: lib/Analysis/ScalarEvolution.cpp:8304
+
+  APInt D = B*B - 4*A*C;
+
----------------
I think you can overflow here, and a few other places where you multiply numbers.  Not sure exactly how many more bits you need.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:10538
+        if (Range.contains(RootVal->getValue())) {
           // The next iteration must be out of the range...
           ConstantInt *NextVal =
----------------
Would it make sense to just return getCouldNotCompute() here?  Given the SolveQuadraticEquation() API, if RootVal is in the range, something overflowed, I think.


Repository:
  rL LLVM

https://reviews.llvm.org/D48283