From ca79221be4b034d71c692ba59d11ad1fc3e18fa8 Mon Sep 17 00:00:00 2001
From: Roman Lebedev <lebedev.ri@gmail.com>
Date: Tue, 3 Nov 2020 13:34:38 +0300
Subject: [PATCH] [InstCombine] Negator: - (C - %x) --> %x - C (PR47997)

This relaxes one-use restriction on that `sub` fold,
since apparently the addition of Negator broke
preexisting `C-(C2-X) --> X+(C-C2)` (with C=0) fold.
---
 .../InstCombine/InstCombineNegator.cpp          | 17 ++++++++++-------
 test/Transforms/InstCombine/icmp.ll             |  4 +---
 test/Transforms/InstCombine/sub-of-negatible.ll |  6 +++---
 3 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/lib/Transforms/InstCombine/InstCombineNegator.cpp b/lib/Transforms/InstCombine/InstCombineNegator.cpp
index b321eab01d7..5a72547ce51 100644
--- a/lib/Transforms/InstCombine/InstCombineNegator.cpp
+++ b/lib/Transforms/InstCombine/InstCombineNegator.cpp
@@ -219,19 +219,22 @@ LLVM_NODISCARD Value *Negator::visitImpl(Value *V, unsigned Depth) {
     break; // Other instructions require recursive reasoning.
   }
 
+  if (I->getOpcode() == Instruction::Sub &&
+      (I->hasOneUse() || (isa<Constant>(I->getOperand(0)) &&
+                          !isa<ConstantExpr>(I->getOperand(0))))) {
+    // `sub` is always negatible.
+    // However, only do this either if the old `sub` doesn't stick around, or
+    // it was subtracting from a constant. Otherwise, this isn't profitable.
+    return Builder.CreateSub(I->getOperand(1), I->getOperand(0),
+                             I->getName() + ".neg");
+  }
+
   // Some other cases, while still don't require recursion,
   // are restricted to the one-use case.
   if (!V->hasOneUse())
     return nullptr;
 
   switch (I->getOpcode()) {
-  case Instruction::Sub:
-    // `sub` is always negatible.
-    // But if the old `sub` sticks around, even thought we don't increase
-    // instruction count, this is a likely regression since we increased
-    // live-range of *both* of the operands, which might lead to more spilling.
-    return Builder.CreateSub(I->getOperand(1), I->getOperand(0),
-                             I->getName() + ".neg");
   case Instruction::SDiv:
     // `sdiv` is negatible if divisor is not undef/INT_MIN/1.
     // While this is normally not behind a use-check,
diff --git a/test/Transforms/InstCombine/icmp.ll b/test/Transforms/InstCombine/icmp.ll
index 27b85aae180..124195a8469 100644
--- a/test/Transforms/InstCombine/icmp.ll
+++ b/test/Transforms/InstCombine/icmp.ll
@@ -3831,9 +3831,7 @@ define i1 @pr47997(i32 %arg) {
 ; CHECK-NEXT:    store i32 [[I]], i32* @x, align 4
 ; CHECK-NEXT:    [[I1:%.*]] = sub nsw i32 1, [[ARG]]
 ; CHECK-NEXT:    store i32 [[I1]], i32* @y, align 4
-; CHECK-NEXT:    [[I2:%.*]] = sub nsw i32 0, [[I1]]
-; CHECK-NEXT:    [[I3:%.*]] = icmp eq i32 [[I]], [[I2]]
-; CHECK-NEXT:    ret i1 [[I3]]
+; CHECK-NEXT:    ret i1 true
 ;
 bb:
   %i = add nsw i32 %arg, -1
diff --git a/test/Transforms/InstCombine/sub-of-negatible.ll b/test/Transforms/InstCombine/sub-of-negatible.ll
index 40f51d2c75a..bd8e9d23d73 100644
--- a/test/Transforms/InstCombine/sub-of-negatible.ll
+++ b/test/Transforms/InstCombine/sub-of-negatible.ll
@@ -195,10 +195,10 @@ define i8 @neg_of_sub_from_constant(i8 %x) {
 
 define i8 @neg_of_sub_from_constant_multi_use(i8 %x) {
 ; CHECK-LABEL: @neg_of_sub_from_constant_multi_use(
-; CHECK-NEXT:    [[S:%.*]] = sub i8 42, [[X:%.*]]
+; CHECK-NEXT:    [[S_NEG:%.*]] = add i8 [[X:%.*]], -42
+; CHECK-NEXT:    [[S:%.*]] = sub i8 42, [[X]]
 ; CHECK-NEXT:    call void @use8(i8 [[S]])
-; CHECK-NEXT:    [[R:%.*]] = sub i8 0, [[S]]
-; CHECK-NEXT:    ret i8 [[R]]
+; CHECK-NEXT:    ret i8 [[S_NEG]]
 ;
   %s = sub i8 42, %x
   call void @use8(i8 %s)