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

[Dinesh Dwivedi]

Thanks for review and comments. I have replied them inline.

================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:398
@@ -397,1 +397,3 @@
 
+/// getIdentityValue - This function returns identity value for given opcode,
+/// which can be used to factor patterns like
----------------
Rafael Ávila de Espíndola wrote:
> Nit: don't duplicate the function name in the comment.
updated.

================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:415
@@ +414,3 @@
+  }
+  return nullptr;
+}
----------------
Rafael Ávila de Espíndola wrote:
> You have both a default case with a return and a return after the switch.
> 
> Given that we only handle Mul, it is probably better to write this as an if for now. When we add support for other opcodes it is easy to go back to an switch.
> 
updated.

================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:418
@@ +417,3 @@
+
+/// getBinOpsForFactorization - This function factors binary ops which can be
+/// combined using distributive laws. This also factor SHL as MUL
----------------
Rafael Ávila de Espíndola wrote:
> same
updated.

================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:442
@@ +441,3 @@
+
+/// tryFactorization - This tries to simplify binary operations by factorizing
+/// out common terms (eg "(A*B)+(A*C)" -> "A*(B+C)").
----------------
Rafael Ávila de Espíndola wrote:
> here too.
updated.

================
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
----------------
Jingyue Wu wrote:
> Do we still need to try right distribution if left distribution already succeeds? 
No. We should not. I have updated code to check 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) {
----------------
Jingyue Wu wrote:
> A question: any chance to preserve NSW/NUW for TopLevelOpcode? 
I will try to do that in next patches.

================
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);
----------------
Jingyue Wu wrote:
> I think it's better to write: 
> ```
> if (BinaryOperator *Op0 = dyn_cast<BinaryOperator>(LHS)) {
>   if (isa<OverflowingBinaryOperator>(Op0)) {
>     HasNSW &= Op0->hasNoSignedWrap();
> }
> ```
updated.

================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:515
@@ +514,3 @@
+
+      cast<BinaryOperator>(SimplifiedInst)->setHasNoSignedWrap(HasNSW);
+    }
----------------
Jingyue Wu wrote:
> 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. 
We have already check if 'isa<OverflowingBinaryOperator>(SimplifiedInst)' so casting SimplifiedInst to BinaryOperator should be safe.

================
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.
----------------
Jingyue Wu wrote:
> `(A op' B) op (C)`? 
updated.

================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:552
@@ +551,3 @@
+  if (Value *V = tryFactorization(Builder, DL, I, RHSOpcode, LHS,
+                                  getIdentityValue(LHSOpcode, RHS), C, D))
+    return V;
----------------
Rafael Ávila de Espíndola wrote:
> This should be
> 
>   if (Value *V = tryFactorization(Builder, DL, I, RHSOpcode, LHS,
>                                   getIdentityValue(LHSOpcode, LHS), C, D))
> 
> no?
> 
updated code. last minute copy paste issue. it should be
  getIdentityValue(RHSOpcode, LHS)

I will be more carefull to avoid these mistakes.

http://reviews.llvm.org/D3799