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

[Dinesh Dwivedi]

Thanks for review. I ahve added comments inline.

================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:400
@@ +399,3 @@
+// e.g. (X * 2) + X ==> (X * 2) + (X * 1) ==> X * (2 + 1)
+Value *FactorUsingIdentityValue(Instruction::BinaryOps OpCode, Value *V,
+                                Value *&FoldedRHS) {
----------------
Rafael Ávila de Espíndola wrote:
> Please name functions starting with a lowercase letter.
> This function can be static.
updated.

I have kept this funtion so it can be used to get identity values
for other operations e.g. and, or etc. as well.

================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:418
@@ +417,3 @@
+// laws. This also factor SHL as MUL e.g. SHL(X, 2) ==> MUL(X, 4)
+Value *FactorBinaryOps(BinaryOperator *Op, Value *&FoldedRHS,
+                       Instruction::BinaryOps &OpCode) {
----------------
Rafael Ávila de Espíndola wrote:
> lowercase, static.
> 
> It seems a bit strange to return one operand and put the other one in an out value. Also, this function doesn't do the factorization, it just splits the expression.
> 
> Also, the above function for handling 1 could be merged into this (and with that avoid the extra indentation), no? Something like
> 
> // Try to split Op0 and Op1 into (A op B) and (C op D) with the same operation
> // in both. This handles cases like matching a shift as a mul and B or D being
> // implicitly 1.
> static bool matchForFactorization(const BinaryOperator *Op0,
>                                   const BinaryOperator *Op1,
>                                   Instruction::BinaryOps &OpCode, Value *&A,
>                                   Value *&B, Value *&C, Value *&D)
> 
Update function name and added static.
I have to keep both function separate to try to explore optimization, jingyue
has mentioned in his comment.

I have changed function parameter and return value. Let me know if it looks ok.



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

================
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);
----------------
Jingyue Wu wrote:
> 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. 
updated. New patch takes care of all except LHS (identity), RHS (identity).
it is getting handled before SimplifyUsingDistributiveLaws called.

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

http://reviews.llvm.org/D3799