[PATCH] D56679: [InstCombine] Don't undo 0 - (X * Y) canonicalization when combining subs.

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In D56679#1356651 <https://reviews.llvm.org/D56679#1356651>, @lebedev.ri wrote:

> We are sure that we want to break the cycle here, and not at the other end?
>  **This** results in the optimal IR?


I think it's fine to break the cycle here, but we should address the problem pattern directly - we want to avoid the transform on a constant expression rather than a plain constant.

So something like this (the variable named "CI" is misleading since that usually refers to "ConstantInt"):

  Index: lib/Transforms/InstCombine/InstCombineAddSub.cpp
  ===================================================================
  --- lib/Transforms/InstCombine/InstCombineAddSub.cpp	(revision 351077)
  +++ lib/Transforms/InstCombine/InstCombineAddSub.cpp	(working copy)
  @@ -1649,7 +1649,6 @@
       // X - A*-B -> X + A*B
       // X - -A*B -> X + A*B
       Value *A, *B;
  -    Constant *CI;
       if (match(Op1, m_c_Mul(m_Value(A), m_Neg(m_Value(B)))))
         return BinaryOperator::CreateAdd(Op0, Builder.CreateMul(A, B));
   
  @@ -1656,8 +1655,8 @@
       // X - A*CI -> X + A*-CI
       // No need to handle commuted multiply because multiply handling will
       // ensure constant will be move to the right hand side.
  -    if (match(Op1, m_Mul(m_Value(A), m_Constant(CI)))) {
  -      Value *NewMul = Builder.CreateMul(A, ConstantExpr::getNeg(CI));
  +    if (match(Op1, m_Mul(m_Value(A), m_Constant(C))) && !isa<ConstantExpr>(C)) {
  +      Value *NewMul = Builder.CreateMul(A, ConstantExpr::getNeg(C));
         return BinaryOperator::CreateAdd(Op0, NewMul);
       }
     }

We needed a similar fix here:
rL333610 <https://reviews.llvm.org/rL333610>


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

https://reviews.llvm.org/D56679