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

[Rafael Ávila de Espíndola]

Looks a lot better, thanks.

================
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) {
----------------
Please name functions starting with a lowercase letter.
This function can be static.

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

http://reviews.llvm.org/D3799