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

[Allen zhong via Phabricator via llvm-commits]

Allen added inline comments.


================
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))
----------------
spatel wrote:
> 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?
Thanks, I restrict to **mul ** as some performance regression of logic combination tests


================
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 &&
----------------
spatel wrote:
> Is this change independent of the one above? If so, we should have separate patches and tests, so it is clear what this does.
Yes, for case (t-1)+(5*t) -> (t*5)+(t-1) -> (t*5+t)-1
  * here is the 1st step: (t-1)+(5*t) -> (t*5)+(t-1) 
  * above transform is the 2ns step: (t*5)+(t-1) -> (t*5+t)-1


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

https://reviews.llvm.org/D136623