[PATCH] Fixing inst-combine not to drops nsw when combining adds into mul (PR19263)

[Jingyue Wu]

================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:481
@@ +480,3 @@
+  // Does "(X op Y) op' Z" always equal "(X op' Z) op (Y op' Z)"?
+  if (RightDistributesOverLeft(TopLevelOpcode, InnerOpcode))
+    // Does the instruction have the form "(A op' B) op (C op' B)" or, in the
----------------
Do we still need to try right distribution if left distribution already succeeds? 

================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:494
@@ +493,3 @@
+      if (!V && LHS->hasOneUse() && RHS->hasOneUse())
+        V = Builder->CreateBinOp(TopLevelOpcode, A, C, LHS->getName());
+      if (V) {
----------------
A question: any chance to preserve NSW/NUW for TopLevelOpcode? 

================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:508
@@ +507,3 @@
+      bool HasNSW = I.hasNoSignedWrap();
+      BinaryOperator *Op0 = dyn_cast<BinaryOperator>(LHS);
+      BinaryOperator *Op1 = dyn_cast<BinaryOperator>(RHS);
----------------
I think it's better to write: 
```
if (BinaryOperator *Op0 = dyn_cast<BinaryOperator>(LHS)) {
  if (isa<OverflowingBinaryOperator>(Op0)) {
    HasNSW &= Op0->hasNoSignedWrap();
}
```

================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:515
@@ +514,3 @@
+
+      cast<BinaryOperator>(SimplifiedInst)->setHasNoSignedWrap(HasNSW);
+    }
----------------
I may be over-concerned, but is this cast dangerous? CreateBinOp returns a ConstantExpr if its operands are both Constant. Note that ConstantInt op ConstantInt is not always foldable, e.g., ptrtoint (a global variable) + 5. 

================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:543
@@ +542,3 @@
+
+  // The instruction has the form "(A op' B) op (A)".  Try to factorize common
+  // term.
----------------
`(A op' B) op (C)`? 

================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:466
@@ +465,3 @@
+      bool InnerCommutative = Instruction::isCommutative(LHSOpcode);
+      // Does "X op' (Y op Z)" always equal "(X op' Y) op (X op' Z)"?
+      if (LeftDistributesOverRight(LHSOpcode, TopLevelOpcode))
----------------
Dinesh Dwivedi wrote:
> Dinesh Dwivedi wrote:
> > Jingyue Wu wrote:
> > > Dinesh Dwivedi wrote:
> > > > Jingyue Wu wrote:
> > > > > I feel the logic of this IF and next IF (if (RightDistributesOverLeft) a bit difficult to follow. There is also some copy and paste. What about extracting the check on InnerCommutative out? Something like:
> > > > > ```
> > > > > try(a, b, c, d);
> > > > > if (op' is commutative) {
> > > > >   try(b, a, c, d);
> > > > >   try(a, b, d, c);
> > > > >   try(b, a, c, d);
> > > > > }
> > > > > ```
> > > > > By doing this, inside each "try", we don't need to think about whether op' is commutative. 
> > > > > 
> > > > > What do you think?
> > > > Updated patch to handle this. We can not just depend on isCommutative as
> > > > there are oparation which are not commutative but still can be distributed e.g.
> > > > "(X + Y) / Z = X/Z + Y/Z"
> > > > 
> > > > We do not handle this now but we might want to do it in future.
> > > I think the first "try" in my pseudo-code handle this case. Right? 
> > > 
> > > While "try" doesn't swap a and b or c and d, it considers both LeftDistributesOverRight and RightDistributesOverLeft. Therefore, it is able to convert "a / b + c / d" to "(a + c) / d". 
> > Yes, if we considers both LeftDistributesOverRight and RightDistributesOverLeft, it handles all cases. 
> > But then I think current code is handling all cases more efficiently. 
> > 
> > If left distributes over right and inner op is commutative, then right will also distribute over left 
> > too and vice versa. So Checking following 2 combinations if left distributes over right
> >   try(a, b, c, d);
> >   if (op' is commutative) {
> >     try(a, b, d, c);
> >   }
> > and following 2 combination if right distributes ove left should be sufficient.
> >   try(a, b, c, d);
> >   if (op' is commutative) {
> >     try(a, b, d, c);
> >   }
> > 
> > Because for inner commutative operation
> > 
> > if try(a, b, c, d) fails for LeftDistributesOverRight then try(b, a, d, c) will fail for RightDistributesOverLeft
> > if try(a, b, c, d) fails for RightDistributesOverLeft then try(b, a, d, c) will fail for LeftDistributesOverRight
> > 
> > similarly
> > 
> > if try(b, a, c, d) fails for LeftDistributesOverRight then try(a, b, d, c) will fail for RightDistributesOverLeft
> > if try(a, b, c, d) fails for RightDistributesOverLeft then try(a, b, d, c) will fail for LeftDistributesOverRight
> > 
> > Am I missing something?
> Please read last line in previous reply as
> if try(b, a, c, d) fails for RightDistributesOverLeft then try(a, b, d, c) will fail for LeftDistributesOverRight
Good point. I agree
```
try(a, b, c, d);
if (op' is commutative) {
  try(a, b, d, c);
}
```
is enough.

http://reviews.llvm.org/D3799