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

[fhahn]

Summary

Otherwise instcombine gets stuck in a cycle. The canonicalization was
added in D55961.

This patch fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=12400

[lebedev.ri]

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

test/Transforms/InstCombine/mul.ll

505–507

Can you please add an explanation here?

[spatel]

In 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

[fhahn]

Thanks, added check for ConstantExpr.

[fhahn]

test/Transforms/InstCombine/mul.ll

448

I re-run update_test_checks.py and it changed those lines. Let me know if I should undo the unrelated changes.

[spatel]

LGTM

lib/Transforms/InstCombine/InstCombineAddSub.cpp

1655

Please change the 'CI' to 'C' in this comment to match the updated code.

[shchenz]

Thanks for the fix.

[lebedev.ri]

Looks good, no further comments from me.

[fhahn]

Committed in rL351187, thanks!

lib/Transforms/InstCombine/InstCombineAddSub.cpp

if (match(Op1, m_SExt(m_Value(Y))) &&
       BinaryOperator *Add = BinaryOperator::CreateAdd(Op0, Zext);
       Add->setHasNoSignedWrap(I.hasNoSignedWrap());
       return Add;
     }
 
     // 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));
 
     // 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);
     }
   }
 
   {
     // ~A - Min/Max(~A, O) -> Max/Min(A, ~O) - A
     // ~A - Min/Max(O, ~A) -> Max/Min(A, ~O) - A
     // Min/Max(~A, O) - ~A -> A - Max/Min(A, ~O)

test/Transforms/InstCombine/mul.ll

 ; CHECK-NEXT:    ret i128 [[MUL]]
 ;
   %mul = mul nsw i128 %X, 2
   ret i128 %mul
 }
 
 define i32 @test_mul_canonicalize_op0(i32 %x, i32 %y) {
 ; CHECK-LABEL: @test_mul_canonicalize_op0(
-; CHECK-NEXT:    [[MUL:%.*]] = mul i32 [[X:%.*]], [[Y:%.*]]
-; CHECK-NEXT:    [[NEG:%.*]] = sub i32 0, [[MUL]]
-; CHECK-NEXT:    ret i32 [[NEG]]
+; CHECK-NEXT:    [[TMP1:%.*]] = mul i32 [[X:%.*]], [[Y:%.*]]
+; CHECK-NEXT:    [[MUL:%.*]] = sub i32 0, [[TMP1]]
+; CHECK-NEXT:    ret i32 [[MUL]]
 ;
   %neg = sub i32 0, %x
   %mul = mul i32 %neg, %y
   ret i32 %mul
 }
 
 define i32 @test_mul_canonicalize_op1(i32 %x, i32 %z) {
 ; CHECK-LABEL: @test_mul_canonicalize_op1(
 ; 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]]
+; CHECK-NEXT:    [[TMP1:%.*]] = mul i32 [[Y]], [[X:%.*]]
+; CHECK-NEXT:    [[MUL:%.*]] = sub i32 0, [[TMP1]]
+; CHECK-NEXT:    ret i32 [[MUL]]
 ;
-  %y = mul i32 %z, 3 
+  %y = mul i32 %z, 3
   %neg = sub i32 0, %x
   %mul = mul i32 %y, %neg
   ret i32 %mul
 }
 
 define i32 @test_mul_canonicalize_nsw(i32 %x, i32 %y) {
 ; CHECK-LABEL: @test_mul_canonicalize_nsw(
-; CHECK-NEXT:    [[MUL:%.*]] = mul i32 [[X:%.*]], [[Y:%.*]]
-; CHECK-NEXT:    [[NEG:%.*]] = sub i32 0, [[MUL]]
-; CHECK-NEXT:    ret i32 [[NEG]]
+; CHECK-NEXT:    [[TMP1:%.*]] = mul i32 [[X:%.*]], [[Y:%.*]]
+; CHECK-NEXT:    [[MUL:%.*]] = sub i32 0, [[TMP1]]
+; CHECK-NEXT:    ret i32 [[MUL]]
 ;
   %neg = sub nsw i32 0, %x
   %mul = mul nsw i32 %neg, %y
   ret i32 %mul
 }
 
 define <2 x i32> @test_mul_canonicalize_vec(<2 x i32> %x, <2 x i32> %y) {
 ; CHECK-LABEL: @test_mul_canonicalize_vec(
-; CHECK-NEXT:    [[MUL:%.*]] = mul <2 x i32> [[X:%.*]], [[Y:%.*]]
-; CHECK-NEXT:    [[NEG:%.*]] = sub <2 x i32> zeroinitializer, [[MUL]]
-; CHECK-NEXT:    ret <2 x i32> [[NEG]]
+; CHECK-NEXT:    [[TMP1:%.*]] = mul <2 x i32> [[X:%.*]], [[Y:%.*]]
+; CHECK-NEXT:    [[MUL:%.*]] = sub <2 x i32> zeroinitializer, [[TMP1]]
+; CHECK-NEXT:    ret <2 x i32> [[MUL]]
 ;
   %neg = sub <2 x i32> <i32 0, i32 0>, %x
   %mul = mul <2 x i32> %neg, %y
   ret <2 x i32> %mul
 }
 
 define i32 @test_mul_canonicalize_multiple_uses(i32 %x, i32 %y) {
 ; CHECK-LABEL: @test_mul_canonicalize_multiple_uses(
 ; CHECK-NEXT:    [[NEG:%.*]] = sub i32 0, [[X:%.*]]
 ; CHECK-NEXT:    [[MUL:%.*]] = mul i32 [[NEG]], [[Y:%.*]]
 ; CHECK-NEXT:    [[MUL2:%.*]] = mul i32 [[MUL]], [[NEG]]
 ; CHECK-NEXT:    ret i32 [[MUL2]]
 ;
   %neg = sub i32 0, %x
   %mul = mul i32 %neg, %y
   %mul2 = mul i32 %mul, %neg
   ret i32 %mul2
 }
+
+@X = global i32 5
+
+define i64 @test_mul_canonicalize_neg_is_not_undone(i64 %L1) {
+; Check we do not undo the canonicalization of 0 - (X * Y), if Y is a constant
+; expr.
+; CHECK-LABEL: @test_mul_canonicalize_neg_is_not_undone(
+; CHECK-NEXT:    [[TMP1:%.*]] = mul i64 [[L1:%.*]], ptrtoint (i32* @X to i64)
+; CHECK-NEXT:    [[B4:%.*]] = sub i64 0, [[TMP1]]
+; CHECK-NEXT:    ret i64 [[B4]]
+;
+  %v1 = ptrtoint i32* @X to i64
+  %B8 = sub i64 0, %v1
+  %B4 = mul i64 %B8, %L1
+  ret i64 %B4
+}