[PATCH] Added instruction combine to transform few more negative values addition to subtraction (Part 2)

[Jingyue Wu]

================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1001
@@ +1000,3 @@
+           Value *NewOr = Builder->CreateOr(Z, ~(*C1));
+           return Builder->CreateSub(RHS, NewOr, "", IHasNUW, IHasNSW);
+         }
----------------
Dinesh Dwivedi wrote:
> Jingyue Wu wrote:
> > Dinesh Dwivedi wrote:
> > > Jingyue Wu wrote:
> > > > I doubt using IHasNUW and IHasNSW is correct due to the logic at Line 985-986. The logic there reassociates
> > > > ```
> > > > I = (x + 1) + ((z & c) ^ c)
> > > > ```
> > > > into
> > > > ```
> > > > I' = x + (((z & c) ^ c) + 1)
> > > > ```
> > > > So, the toplevel "+" in I' may not inherit the NSW/NUW of the toplevel "+" in I. 
> > > > 
> > > > Essentially, (a + 1) +nsw b != a +nsw (b + 1).  Does that make sense to you? 
> > > code here is changing 
> > >   I = (x + 1) + ((z & c) ^ c)
> > > to 
> > >   I = x - (z & c)
> > > I assumed that if first version of I has NUW/ NSW, it is applied to second
> > > version too. I am not shifting those flags from one instruction to other. Basically
> > > this patch is not re-associating anything. It just look for pattern and if found,
> > > transforms I from one form to another.
> > Although your code doesn't actually reassociate the expression, it does that conceptually. 
> > 
> > For example, are you saying it's safe to transform
> > ```
> > I = (x + 1) +nsw ((z & c) ^ c)
> > ```
> > to
> > ```
> > I = x -nsw (z | ~c)
> > ```
> > ?
> > 
> > Consider all integers are of type i4, and x = 0111, z = 0000, c = 0111. The first version finally computes
> > ```
> > 1000 + 0111
> > ```
> > which does not sign-overflow. However, the second version finally computes
> > ```
> > 0111 - 1000 = 7 - (-8)
> > ```
> > which sign-overflows.
> I still have doubts about this. I think NSW belong to oparator and not operand. for above, example
> my patch will not put NSW in '(z | ~c)' or in 'I'.
> 
> If we have something like
>   I = nsw ((x + 1) + ((z & c) ^ c))
> then result will be
>   I = nsw (x - (z | ~c))
> 
> My patch does not upadate any flag for (x + 1) or ((z & c) ^ c). 
> '(z | ~c)' will be new instruction without any flag.
> 
> In the Z3 link above, I have proved that ((x + 1) + ((z & c) ^ c)) == (x - (z | ~c))
> so if first version of 'I' is overflowing, second will too and if first doen't, neither do
> second.
> 
> But I am not sure about above logic. If we have to take care of that, there are 2 options
> 1. to remove code for setting for nsw in instruction after transform, but this may loose nsw and opportunities for further optimization.
> 2. to add check 'if I has NSW return nullptr' i.e. do not transform in case I has nsw as add nsw is not reassociative.
> 
> I am not sure which one is better.
>   
Hi Dinesh, 

Sorry for the delay. I feel I need to first confirm my understanding of sign-overflow which may be wrong. I just sent a question to llvmdev. 

Thanks,
Jingyue

http://reviews.llvm.org/D4209