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

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:8256
+/// Find the smallest non-negative number n, such that after n iterations
+/// the quadratic chrec {L,+,M,+,N} equals 0 or changes sign.
+static Optional<const SCEVConstant *>
----------------
Please clarify what happens if the addrec has signed overflow.  Please document the conditions under which the algorithm fails to find a solution.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:8285
+    // In i1, 1 is the same as -1, so sign-extending (i1 1) would actually
+    // produce -1 in the larger type.
+    N = N.zext(NewWidth);
----------------
The algorithm should still work even if you treat "i1 true" as -1, I think.  Actually, you end up solving the exact same equation, because of the `A.isNegative()` check.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:10632
+        if (Range.contains(RootVal->getValue()))
           return SE.getCouldNotCompute(); // Something strange happened
 
----------------
Please make sure we have a testcase that hits this "getCouldNotCompute".


================
Comment at: lib/Analysis/ScalarEvolution.cpp:10641
+          return R;
         return SE.getCouldNotCompute(); // Something strange happened
     }
----------------
Please make sure we have a testcase that hits this "getCouldNotCompute".


================
Comment at: test/Analysis/ScalarEvolution/solve-quadratic-overflow.ll:2
+; RUN: opt -analyze -scalar-evolution -S < %s | FileCheck %s
+
+; CHECK: Loop %b1: backedge-taken count is 255
----------------
Please integrate this into the other testcase, with the same comments/CHECK lines.


================
Comment at: test/Analysis/ScalarEvolution/solve-quadratic.ll:1
+; RUN: opt -analyze -scalar-evolution -S -debug-only=scalar-evolution < %s 2>&1 | FileCheck %s
+
----------------
debug-only needs "REQUIRES: asserts".


Repository:
  rL LLVM

https://reviews.llvm.org/D48283