[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

1656–1657

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 (Op1->hasOneUse()) {
 
     // 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
+    // We have to make sure we do not undo the 0 - (X * Y) canonicalization.
     // 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)))) {
+    if (!match(Op0, m_ZeroInt()) && match(Op1, m_Mul(m_Value(A), m_Constant(CI)))) {
       Value *NewMul = Builder.CreateMul(A, ConstantExpr::getNeg(CI));
       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

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]]
 ;
   %neg = sub i32 0, %x
   %mul = mul i32 %neg, %y
   ret i32 %mul
 }
 

 ; 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-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
+}