[PATCH] D37209: [LSR] Fix Shadow IV in case of integer overflow

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added inline comments.


================
Comment at: lib/Transforms/Scalar/LoopStrengthReduce.cpp:2034
+    const SCEVAddRecExpr *PS = dyn_cast<SCEVAddRecExpr>(SE.getSCEV(PH));
+    if (!PS || !PS->hasNoSelfWrap()) continue;
+
----------------
reames wrote:
> reames wrote:
> > I don't think this is the correct check.  Specifically, this check allows overflow as long as we can guarantee that the value never reaches it's starting value.  I think you actually want hasNoUnsignedWrap() && hasNoSignedWrap()
> > 
> > Consider:
> > unsigned i = 1
> > do {
> >   i += INT_MAX; 
> >   d += i;
> > } while ( (signed int)i > 0)
> Giving a bit more detail, signed wrap is definitely a problem which needs prevented because double looses precision for maximum bitwidth and produces a different answer for smaller bit widths.  consider signed_max<src_type> + 1.  This should equal signed_min<src_type>, but may not with double arithmetic.  
> 
> Unsigned wrap is less clear to me.  I don't have a counter example yet, but I'm suspicious that there is one.  If we allowed negative steps, we'd clearly have the corresponding underflow problem to the signed wrap one just mentioned, but with the restriction to positive steps, I'm not finding one.
> 
> Actually, here's an obvious one.
> for (unsigned int16_t = UINT16_MAX; i != UINT_MAX-1; i++) { (double)i };
> 
> UINT16_MAX + 1 should be 0.  Since that fits within the signed domain of a double, we'd get (int)UINT16_MAX)+1.  
> 
> 
> 
>  
Thanks for pointing it out, it should indeed be nuw/nsw wraps.

As for unsigned, the situation is basically the same as with signed even without negative steps. Double is able to store values that are greater than UINT_MAX, and on conversion from double to int they will be rounded to nearest (UINT_MAX, apparently) while in int they will be overflown values that are less than it.


https://reviews.llvm.org/D37209