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

[Dinesh Dwivedi]

Sorry for delay in reply. I have added comments inline.

================
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:
> 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! 
yes, it handled in re-association. For above case, I got following IR at -O3 (with/ without my patch)
  %and = and i32 %z, 123
  %xor = xor i32 %and, 123
  %add1 = add i32 %w, 2

Moreover, checks in my patch will not match in case of above test case as they will match only if
there are only one '+ 1' and one pattern for XOR(AND())

Following code
  LHS = ((w & 126) ^ 126) + 1;
  RHS = ((z & 123) ^ 123) + 1;
gets transformed to
  %and = and i32 %w, 126
  %and1 = and i32 %z, 123
  %xor2 = xor i32 %and1, 123
  %add3 = sub i32 128, %and
i.e. only one patten is transformed. This will be fixed with my next patch, where I am planning to 
use current logic to transform SUB instructions as well.

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

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

http://reviews.llvm.org/D4209