[PATCH] D37265: [SCEV] Ensure ScalarEvolution::createAddRecFromPHIWithCastsImpl properly handles out of range truncations of the start and accum values

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev requested changes to this revision.
mkazantsev added inline comments.
This revision now requires changes to proceed.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:4486
+    Predicates.push_back(AddRecPred);
+  }
 
----------------
Assert that otherwise it is a `SCEVConstant`, or you expect it being something else?


================
Comment at: lib/Analysis/ScalarEvolution.cpp:4507
+  //#define OPTION_1
+  auto PredIsKnownFalse = [&](const SCEV *Expr,
+                              const SCEV *ExtendedExpr) -> bool {
----------------
Please write a comment what this function is supposed to do. Semantically it looks like you mean `isKnownNonEqual`.


================
Comment at: test/Analysis/ScalarEvolution/overflow-addrec.ll:1
+; RUN: opt -loop-vectorize -S < %s | FileCheck %s
+
----------------
I don't think that `-loop-vectorize` is the best way to test SCEV (even though we have a couple of such files in this folder). Is there a way to see the impact of your changes using `opt -analyze -scalar-evolution`? You can keep this test set (e.g. in vectorization), but in `test/Analysis/ScalarEvolution` I'd rather see something that checks SCEVs.



================
Comment at: test/Analysis/ScalarEvolution/overflow-addrec.ll:18
+
+bb:
+; CHECK: bb:
----------------
Please rename blocks into something like `entry/loop/exit` to make it more understandable.


================
Comment at: test/Analysis/ScalarEvolution/overflow-addrec.ll:28
+; CHECK: %tmp3 = ashr exact i64 %tmp2, 32
+; CHECK: %tmp4 = add i64 %tmp3, -9223372036854775808
+; CHECK: br
----------------
You may add a comment that this value is `INT64_MIN`.


https://reviews.llvm.org/D37265