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

[Jingyue Wu]

Hi Dinesh, 

It looks pretty good overall. Thanks for working on this! Please find my comments inlined.

================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:445
@@ +444,3 @@
+
+  Value *A = nullptr, *B = nullptr, *C = nullptr, *D = nullptr;
+  Instruction::BinaryOps LHSOpcode = Instruction::BinaryOpsEnd;
----------------
I'd move these definitions closer to their uses. e.g.,
  Value *B = nullptr, *D = nullptr;
  Value *A = FactorBinaryOps(Op0, B, LHSOpcode);

================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:452
@@ -408,3 +451,3 @@
 
-  // Factorization.
-  if (Op0 && Op1 && Op0->getOpcode() == Op1->getOpcode()) {
+  A = FactorBinaryOps(Op0, B, LHSOpcode);
+  C = FactorBinaryOps(Op1, D, RHSOpcode);
----------------
Correct me if I'm wrong. I think the current logic seems unable to convert
  %z = mul nsw i32 %x, %y
  %z3 = mul nsw i32 %z, 3
  %z4 = add nsw i32 %z, %z3
to
  %z = mul nsw i32 %x, %y
  %z4 = mul nsw i32 %z, 4
because if %z is in the format of %x * %y, you will not try the identity factorization (i.e., %z * 1). 

If it is indeed a missed opportunity, it may not be too difficult to cover. For example, we can try all four combinations:
# LHS (binary op), RHS (binary op)
# LHS (identity), RHS (binary op)
# LHS (binary op), RHS (identity)
# LHS (identity), RHS (identity) // probably already handled somewhere else?

Anyway, we can do this later. 

================
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))
----------------
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?

http://reviews.llvm.org/D3799