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

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

Further 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:
> 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.  



 


================
Comment at: test/Transforms/LoopStrengthReduce/X86/2008-08-14-ShadowIV.ll:1
 ; RUN: opt < %s -loop-reduce -S -mtriple=x86_64-unknown-unknown | grep "phi double" | count 1
 
----------------
Please convert this test to use filecheck (the automatic check lines should be fine), check that in, and then rebase please.


================
Comment at: test/Transforms/LoopStrengthReduce/X86/2008-08-14-ShadowIV.ll:66
 	tail call void @foo( double %2 ) nounwind
 	add i32 %i.03, 1		; <i32>:3 [#uses=2]
 	tail call i32 (...) @nn( ) nounwind		; <i32>:4 [#uses=1]
----------------
Is this even valid IR?  I don't see a value being assigned here?  Unless this is just wrapping really weirdly?


https://reviews.llvm.org/D37209