[PATCH] D136623: [InstCombine] enable more factorization in SimplifyUsingDistributiveLaws

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:759
+  // term.
+  if (Op0) {
+    if (Value *Ident = getIdentityValue(LHSOpcode, RHS))
----------------
Make formatting changes independently of this patch or just leave it unchanged.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:775
+  // common term for "(A * B) op (C)".
+  if (Op0 && Op1 && LHSOpcode == Instruction::Mul &&
+      TopLevelOpcode == RHSOpcode && Instruction::isCommutative(RHSOpcode))
----------------
It seems odd for this function to specify the exact opcode. Everything else is generalized based on association and distribution laws. Is `mul` really the only opcode where this transform works?


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:780
+        Value *New = Builder.CreateBinOp(RHSOpcode, V, D);
+        if (dyn_cast<OverflowingBinaryOperator>(&I)) {
+          cast<Instruction>(New)->setHasNoSignedWrap(I.hasNoSignedWrap());
----------------
Use plain `cast' if we don't use the return value. Alternatively, give it a name and use it:
  if (auto *OBO = dyn_cast<OverflowingBinaryOperator>(&I)) {
    cast<Instruction>(New)->setHasNoSignedWrap(OBO.hasNoSignedWrap())

How did we guarantee that "New" is an OBO instruction?

If it is safe to propagate no-wrap like this, then we need tests and Alive2 proofs to show it. 


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:787-788
 
-    // The instruction has the form "(A op' B) op (C)".  Try to factorize common
-    // term.
-    if (Op0)
-      if (Value *Ident = getIdentityValue(LHSOpcode, RHS))
-        if (Value *V = tryFactorization(I, LHSOpcode, A, B, RHS, Ident))
-          return V;
-
-    // The instruction has the form "(B) op (C op' D)".  Try to factorize common
-    // term.
-    if (Op1)
-      if (Value *Ident = getIdentityValue(RHSOpcode, LHS))
-        if (Value *V = tryFactorization(I, RHSOpcode, LHS, Ident, C, D))
-          return V;
+  // The instruction has the form "(A op' B) op' (C * D)". See if expanding it
+  // out to "(C * D) op' (A op' B)" results in simplifications.
+  if (Op0 && Op1 && RHSOpcode == Instruction::Mul && A == C &&
----------------
Is this change independent of the one above? If so, we should have separate patches and tests, so it is clear what this does.


================
Comment at: llvm/test/Transforms/InstCombine/add4.ll:131
+
+; (x - 1) - (x * C) --> (1 + C) * x - 1
+define i32 @mul_add_common_factor_commute0(i32 %x) {
----------------
(x + (-1)) + (x * 5) --> (x * 6) + (-1)


================
Comment at: llvm/test/Transforms/InstCombine/add4.ll:144
+
+; (x - 1) - (x * C) --> (1 + C) * x - 1
+define i32 @mul_add_common_factor_commute1(i32 %x) {
----------------
(x * 4) + (x - 1) --> (x * 5) + (-1)

Is this converted to `shl` first?


================
Comment at: llvm/test/Transforms/InstCombine/add4.ll:157
+
+; (x + y) - (x * C) --> (1 + C) * x + y
+define i32 @mul_add_common_factor_commute2(i32 %x, i32 %y) {
----------------
(x + y) + (x * 4) --> (x * 5) + y

Is this converted to `shl` first?


================
Comment at: llvm/test/Transforms/InstCombine/add4.ll:170
+
+; (x + 2) - (x * t) --> (1 + t) * x + 2
+define i32 @mul_add_common_factor_commute3(i32 %x, i32 %t) {
----------------
(x + 2) + (x * t) --> (t + 1) * x + 2


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136623/new/

https://reviews.llvm.org/D136623