[PATCH] D55961: [InstCombine] canonicalize MUL with NEG operand

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

Looks better.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:247-253
+  // -X * Y --> -(X * Y)
+  if (match(Op0, m_OneUse(m_Neg(m_Value(X)))))
+    return BinaryOperator::CreateNeg(Builder.CreateMul(X, Op1));
+
+  // X * -Y --> -(X * Y)
+  if (match(Op1, m_OneUse(m_Neg(m_Value(Y)))))
+    return BinaryOperator::CreateNeg(Builder.CreateMul(Op0, Y));
----------------
This seems correct now, but i //wonder// if it would be better to fold this into a single branch:
```
  // -X * Y --> -(X * Y)
  if (match(&I, m_c_Mul(m_OneUse(m_Neg(m_Value(X))), m_Value(Y))))
    return BinaryOperator::CreateNeg(Builder.CreateMul(X, Y));
```



================
Comment at: llvm/test/Transforms/InstCombine/mul.ll:457-468
+define i32 @test_mul_canonicalize_op1(i32 %x, i32 %z) {
 ; CHECK-LABEL: @test_mul_canonicalize_op1(
-; CHECK-NEXT:    [[NEG:%.*]] = sub i32 0, [[Y:%.*]]
-; CHECK-NEXT:    [[MUL:%.*]] = mul i32 [[NEG]], [[X:%.*]]
-; CHECK-NEXT:    ret i32 [[MUL]]
+; CHECK-NEXT:    [[Y:%.*]] = mul i32 [[Z:%.*]], 3
+; CHECK-NEXT:    [[MUL:%.*]] = mul i32 [[X:%.*]], [[Y:%.*]]
+; CHECK-NEXT:    [[NEG:%.*]] = sub i32 0, [[MUL]]
+; CHECK-NEXT:    ret i32 [[NEG]]
 ;
----------------
I almost complained that this is a regression.
Can you please split the NFC test update into a separate preparatory differential,
and only *regenerate* the check lines here?


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

https://reviews.llvm.org/D55961