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

[Dinesh Dwivedi]

================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1001
@@ +1000,3 @@
+           Value *NewOr = Builder->CreateOr(Z, ~(*C1));
+           return Builder->CreateSub(RHS, NewOr, "", IHasNUW, IHasNSW);
+         }
----------------
Jingyue Wu wrote:
> 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
Now I am too not sure about nsw flag here. I am planning to use Z3 to verify code keeping/ setting 
NSW/ NUW flags. For now, I am removing code setting these flags from the patch.

I will send another patch once I do understand NSW/ NUW.

http://reviews.llvm.org/D4209