[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 a comment.
This revision now requires changes to proceed.

Code part looks ok for me (with comments addressed). I propose you to add a C++ unit test.



================
Comment at: lib/Analysis/ScalarEvolution.cpp:4478
+  //  If PHISCEV is a constant, then P1 degenerates into P2 or P3, so we don't
+  //  add
+  // P1.
----------------
Please align this comment.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:4487
+  } else
+    assert(dyn_cast<SCEVConstant>(PHISCEV) != nullptr &&
+           "Expected constant SCEV");
----------------
  assert(isa<SCEVConstant>(PHISCEV) && ...)


================
Comment at: lib/Analysis/ScalarEvolution.cpp:4516
+// OPTION to be removed before landing.
+// To the reviewers: Which version do we want to use?
+//  I suspect that it's highly unlikely that anything other than
----------------
For me, this option looks more reasonable.
    return Expr != ExtendedExpr &&
           isKnownPredicate(ICmpInst::ICMP_NE, Expr, ExtendedExpr);



================
Comment at: test/Analysis/ScalarEvolution/overflow-addrec.ll:1
+; RUN: opt -loop-vectorize -S < %s | FileCheck %s
+
----------------
dneilson wrote:
> mkazantsev wrote:
> > 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.
> > 
> I don't know the SCEV code very well -- I'm not really that sure how else this patched function is used/triggered. If there's a way to get at the changed method using just opt -analyze -scalar-evolution, then that'd be great. I'm open to suggestions...
If you know a code example in which running `opt -analyze scalar-evolution` produces different output with and without your patch, you can just add it and check that the changes have happened. But most likely that your function does not affect construction of SCEVs and is only used by other passes as an auxiliary method. In this case I'd propose writing a C++ unit test (see `unittests/Analysis/ScalarEvolutionTest.cpp`). You can construct IR similar to your `.ll` examples and then directly invoke your function and check that the result is what you expect to have with your patch (or it just doesn't crash on attempt).


https://reviews.llvm.org/D37265