[PATCH] D12073: Make ScalarEvolution::isKnownPredicate a little smarter

[hfinkel@anl.gov via llvm-commits]

hfinkel added a comment.

In http://reviews.llvm.org/D12073#226397, @hfinkel wrote:

> In http://reviews.llvm.org/D12073#226385, @sanjoy wrote:
>
> > In http://reviews.llvm.org/D12073#226151, @hfinkel wrote:
> >
> > >
> >
> >
> >
> >
> > > There are now three failing tests, mostly due to IV widening that now does not happen. The new known-predicate results are:
> >
> > > 
> >
> > > test/Transforms/IndVarSimplify/elim-extend.ll:
> >
> > > 
> >
> > > - {%init,+,1}<nsw><%loop> sle {(1 + %init),+,1}<nsw><%loop>
> >
> > > - {(sext i32 %init to i64),+,1}<nsw><%loop> sle {(1 + (sext i32 %init to i64)),+,1}<nsw><%loop>
> >
> > > - {-1,+,1}<nsw><%outerloop> sle {1,+,1}<nuw><%outerloop>
> >
> > > - {-1,+,1}<nsw><%outerloop> sle {1,+,1}<nuw><nsw><%outerloop>
> >
> > > 
> >
> > >   test/Transforms/IndVarSimplify/iv-sext.ll:
> >
> > > - {-1,+,1}<nsw><%bb> sle {1,+,1}<nuw><%bb>
> >
> > > - {-1,+,1}<nsw><%bb> sle {1,+,1}<nuw><nsw><%bb>
> >
> > > 
> >
> > >   test/Transforms/IndVarSimplify/strengthen-overflow.ll:
> >
> > > - {%init,+,1}<nsw><%loop> sle {(1 + %init),+,1}<nsw><%loop>
> >
> > > 
> >
> > >   These look right, but some consumer of the information seems to make a worse no-wrap deductions as a result (thus, this patch is still WIP).
> >
> >
> > I suspect (haven't verified) this is due to SCEV's caching mechanism. You end up creating a "Sext(LHS)" too soon, before proving all of the things needed to show that LHS is nsw.  Then this suboptimal result gets cached.
> >
> > In other words, I suspect the stack trace looks like this (youngest frame at top) initially:
> >
> >   == false because of PendingLoopPredicates, set in frame (A)
> >   getImpliedCond(...)  // ends up looking at VAL
> >   getLoopBackedgeCount(...)
> >   getSignExtend(VAL)  // due to your change
> >   getImpliedCond(...)  // ends up looking at VAL .. (A)
> >   getLoopBackedgeCount(...)
> >   getSignExtend(VAL)
> >   
> >
> > This goes on as
> >
> >     getSignExtend(VAL)  ==> no simplification, and we end up with
> >           UniqueSCEVs[(SignExtend, VAL)] = SignExtendExpr VAL, and
> >   	does not set the <nsw> bit in VAL
> >     getImpliedCond(...)  // ends up looking at VAL .. (A)
> >     getLoopBackedgeCount(...)
> >     getSignExtend(VAL)
> >
> >
> > And then finally the oldest frame returns:
> >
> >   getSignExtend(VAL)  // this returns a widened version of VAL (instead of
> >     an explicit sign extend), and also set VAL's nsw bit
> >   
> >
> > However, the damage has been done since `UniqueSCEVs[(SignExtend, VAL)] == (SignExtendExpr VAL)` and that's the value `ScalarEvolution::getSignExtendExpr`  will return from now on.
> >
> > If you implement your check without actually creating a sign extend, but by directly inferring no-overflow from the nsw or nuw flags, then I suspect this won't happen.
>
>
>


After looking at this more, your hypothesis is correct. We end up recursing on getSignExtendExpr `{%init,+,1}<%loop>`, which is broken by something (PendingLoopPredicates makes sense), and thus we cache a bad answer for the sign extension. Later, we retrieve the bad answer from the cache. Thus the problem. Thanks!


http://reviews.llvm.org/D12073