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

[Dinesh Dwivedi]

Thanks. I have replied inline. I will update code based on your comment on them.

================
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);
----------------
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.

================
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)) {
----------------
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.

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

http://reviews.llvm.org/D4209