[PATCH] D30887: [ScalarEvolution] Predicate implication from operations

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added inline comments.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:8527
+    auto *Zero = getZero(RHS->getType());
+    auto *LLExt = getNoopOrSignExtend(LL, OrigLHS->getType());
+    auto *LRExt = getNoopOrSignExtend(LR, OrigLHS->getType());
----------------
mkazantsev wrote:
> sanjoy wrote:
> > mkazantsev wrote:
> > > sanjoy wrote:
> > > > Can we get what we want here without sign extension?  As I've said below, sign extension can be expensive.
> > > > 
> > > > In fact, it would be surprising if we see `LHS` is not the same as `OrigLHS` since that would mean a `sext (%a + %b)<nsw>` did not get transformed to `(sext %a + sext %b)<nsw>` as per the rule in `ScalarEvolution::getSignExtendExpr`.  That situation is possible, but should be rare.
> > > Why is it rare? We can calculate sdiv i32 %a, %b and than use it in multiple ways, one of them being comparison to an i64 constant. In this case we will see exactly this.
> > Maybe we're misinterpreting each other, but I was specifically talking about this `SCEVAddExpr` case.  That is, I'd be surprised if all of the following are simultaneously true:
> > 
> >  - `LHS` is a `SCEVAddExpr` marked as NSW
> >  - `FoundLHS` was a `SCEVSignExtendExpr` with `LHS` as its operand
> > 
> > because if they were, I'd have expected the sign extend to have been have been commuted to inside the add expression.
> Sorry, I misinterpreted it. Yes, this can be removed, I think.
>> In fact, it would be surprising if we see LHS is not the same as OrigLHS since that would mean a sext (%a + %b)<nsw> did not get transformed to (sext %a + sext %b)<nsw> as per the rule in ScalarEvolution::getSignExtendExpr. That situation is possible, but should be rare.

It is possible indeed, and it lead to a crash on CLang built. I should prohibit it.


https://reviews.llvm.org/D30887