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

[Daniel Neilson via Phabricator via llvm-commits]

dneilson added inline comments.


================
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
----------------
mkazantsev wrote:
> mkazantsev wrote:
> > mkazantsev wrote:
> > > mkazantsev wrote:
> > > > For me, this option looks more reasonable.
> > > >     return Expr != ExtendedExpr &&
> > > >            isKnownPredicate(ICmpInst::ICMP_NE, Expr, ExtendedExpr);
> > > > 
> > > By the way. I grew hesitant about the `Expr != ExtendedExpr` condition. If two SCEVs don't match pointer-wise, it doesn't mean that they are not equal. For example, constant `2` and `2` which is load from a field are equal, but you will have a `SCEVConstant` in one case and a `SCEVUnknown` in another case. I think that the correct option would be
> > >   return isKnownPredicate(ICmpInst::ICMP_NE, Expr, ExtendedExpr);
> > It's not the case in your situation, though. So maybe it's OK, but using just `isKnownPredicate` looks more safe.
> Scratch this, I've re-read the code and realised that what I wrote is bizzare. 
>   return Expr != ExtendedExpr &&
>          isKnownPredicate(ICmpInst::ICMP_NE, Expr, ExtendedExpr);
> is ok.
The pointer comparison seemed odd to me as well, but I was matching the idiom used in other parts of the code that does the same thing. I'm guessing that there's some sort of SCEV caching thing going on behind the scenes, and it's possible to get the same SCEV object from different queries.


https://reviews.llvm.org/D37265