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

[Allen zhong via Phabricator via llvm-commits]

Allen marked 2 inline comments as done and an inline comment as not done.
Allen added inline comments.


================
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());
----------------
spatel wrote:
> 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. 
As we have condition **TopLevelOpcode == RHSOpcode**, so the operand of I and New must be same, this can guard the "New" is an OBO instruction


================
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());
----------------
Allen wrote:
> spatel wrote:
> > 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. 
> As we have condition **TopLevelOpcode == RHSOpcode**, so the operand of I and New must be same, this can guard the "New" is an OBO instruction
I don't try to update the no-wrap to start begin, and will check that if the basic changes is accepted.


================
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 &&
----------------
Allen wrote:
> 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
Ok, I'll separate the function after this patch reviewed to make sure this is the direction.


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

https://reviews.llvm.org/D136623