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

[Jingyue Wu]

================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:980
@@ +979,3 @@
+   // if ONE is on other side, swap
+   if (match(RHS, m_Add(m_Value(X), m_One())))
+     std::swap(LHS, RHS);
----------------
Dinesh Dwivedi wrote:
> Jingyue Wu wrote:
> > If
> > ```
> > LHS = w + 1
> > RHS = ((z & c) ^ c) + 1
> > ```
> > the code swaps LHS and RHS and may miss some optimization opportunities. 
> > 
> > Maybe instead of premature swapping, we can look deeper into both LHS and RHS. What do you think? 
> > 
> > I may be over-concerned, because Reassociate will optimize (a + 1) + (b + 1) to a + (b + 2). I am not sure about the ordering. 
> Yes, I think that should be taken care in Reassociate. I think that is the purpose of canonicalization so that we do not have to check of all permutations of inputs. But if
> you say, I will change it.
Can you run -O3 to verify Reassociate does canonicalize such cases before InstCombine? If so, I am fine with the current code. Thanks! 

================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:998
@@ +997,3 @@
+       // ADD(ADD(X, 1), RHS) == ADD(X, ADD(RHS, 1)) == SUB(RHS, OR(Z, ~C1))
+       else if (!C1->countTrailingZeros()) {
+         if (match(Y, m_And(m_Value(Z), m_APInt(C2))) && (*C1 == *C2)) {
----------------
Dinesh Dwivedi wrote:
> Dinesh Dwivedi wrote:
> > Jingyue Wu wrote:
> > > Jingyue Wu wrote:
> > > > Nit: I prefer
> > > > ```
> > > > C1->countTrailingZeros() == 0
> > > > ```
> > > > because C1->countTrailingZeros() is not a boolean. 
> > > Nit: Does LLVM coding standard say anything about: 
> > > ```
> > > } else if {
> > > ```
> > > or
> > > ```
> > > }
> > > else if {
> > > ```
> > > 
> > > The first style seems more commonly used. 
> > will update it.
> For else if, I have used clang-format on it. But may be because of comments, it left 
> else in next line. I will move comments after else if line, if you say so.
Please do. Sorry for being a little paranoid :) 

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

http://reviews.llvm.org/D4209