[InstCombine] Negator: don't negate multi-use `sub`

While we can do that, it doesn't increase instruction count,
if the old `sub` sticks around then the transform is not only
not a unlikely win, but a likely regression, since we likely
now extended live range and use count of both of the `sub` operands,
as opposed to just the result of `sub`.

As Kostya Serebryany notes in post-commit review in
https://reviews.llvm.org/D68408#1998112
this indeed can degrade final assembly,
increase register pressure, and spilling.

This isn't what we want here,
so at least for now let's guard it with an use check.

diff --git a/llvm/lib/Transforms/InstCombine/InstCombineNegator.cpp b/llvm/lib/Transforms/InstCombine/InstCombineNegator.cpp
index e119a38..42bb748 100644 (file)
--- a/llvm/lib/Transforms/InstCombine/InstCombineNegator.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineNegator.cpp
@@ -152,10 +152,6 @@ LLVM_NODISCARD Value *Negator::visit(Value *V, unsigned Depth) {
 
   // In some cases we can give the answer without further recursion.
   switch (I->getOpcode()) {
-  case Instruction::Sub:
-    // `sub` is always negatible.
-    return Builder.CreateSub(I->getOperand(1), I->getOperand(0),
-                             I->getName() + ".neg");
   case Instruction::Add:
     // `inc` is always negatible.
     if (match(I->getOperand(1), m_One()))
@@ -183,12 +179,37 @@ LLVM_NODISCARD Value *Negator::visit(Value *V, unsigned Depth) {
     }
     break;
   }
+  case Instruction::SExt:
+  case Instruction::ZExt:
+    // `*ext` of i1 is always negatible
+    if (I->getOperand(0)->getType()->isIntOrIntVectorTy(1))
+      return I->getOpcode() == Instruction::SExt
+                 ? Builder.CreateZExt(I->getOperand(0), I->getType(),
+                                      I->getName() + ".neg")
+                 : Builder.CreateSExt(I->getOperand(0), I->getType(),
+                                      I->getName() + ".neg");
+    break;
+  default:
+    break; // Other instructions require recursive reasoning.
+  }
+
+  // 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,
     // let's consider division to be special since it's costly.
-    if (!I->hasOneUse())
-      break;
     if (auto *Op1C = dyn_cast<Constant>(I->getOperand(1))) {
       if (!Op1C->containsUndefElement() && Op1C->isNotMinSignedValue() &&
           Op1C->isNotOneValue()) {
@@ -201,24 +222,9 @@ LLVM_NODISCARD Value *Negator::visit(Value *V, unsigned Depth) {
       }
     }
     break;
-  case Instruction::SExt:
-  case Instruction::ZExt:
-    // `*ext` of i1 is always negatible
-    if (I->getOperand(0)->getType()->isIntOrIntVectorTy(1))
-      return I->getOpcode() == Instruction::SExt
-                 ? Builder.CreateZExt(I->getOperand(0), I->getType(),
-                                      I->getName() + ".neg")
-                 : Builder.CreateSExt(I->getOperand(0), I->getType(),
-                                      I->getName() + ".neg");
-    break;
-  default:
-    break; // Other instructions require recursive reasoning.
   }
 
-  // Rest of the logic is recursive, and if either the current instruction
-  // has other uses or if it's time to give up then it's time.
-  if (!V->hasOneUse())
-    return nullptr;
+  // Rest of the logic is recursive, so if it's time to give up then it's time.
   if (Depth > NegatorMaxDepth) {
     LLVM_DEBUG(dbgs() << "Negator: reached maximal allowed traversal depth in "
                       << *V << ". Giving up.\n");

diff --git a/llvm/test/Transforms/InstCombine/sub-of-negatible.ll b/llvm/test/Transforms/InstCombine/sub-of-negatible.ll
index 83f86ca..5489639 100644 (file)
--- a/llvm/test/Transforms/InstCombine/sub-of-negatible.ll
+++ b/llvm/test/Transforms/InstCombine/sub-of-negatible.ll
@@ -169,10 +169,10 @@ define i8 @t9(i8 %x, i8 %y) {
 }
 define i8 @n10(i8 %x, i8 %y, i8 %z) {
 ; CHECK-LABEL: @n10(
-; CHECK-NEXT:    [[T0_NEG:%.*]] = sub i8 [[X:%.*]], [[Y:%.*]]
-; CHECK-NEXT:    [[T0:%.*]] = sub i8 [[Y]], [[X]]
+; CHECK-NEXT:    [[T0:%.*]] = sub i8 [[Y:%.*]], [[X:%.*]]
 ; CHECK-NEXT:    call void @use8(i8 [[T0]])
-; CHECK-NEXT:    ret i8 [[T0_NEG]]
+; CHECK-NEXT:    [[T1:%.*]] = sub i8 0, [[T0]]
+; CHECK-NEXT:    ret i8 [[T1]]
 ;
   %t0 = sub i8 %y, %x
   call void @use8(i8 %t0)

diff --git a/llvm/test/Transforms/InstCombine/sub.ll b/llvm/test/Transforms/InstCombine/sub.ll
index 1b41492..f6fa797 100644 (file)
--- a/llvm/test/Transforms/InstCombine/sub.ll
+++ b/llvm/test/Transforms/InstCombine/sub.ll
@@ -555,11 +555,11 @@ define i64 @test_neg_shl_sub(i64 %a, i64 %b) {
 
 define i64 @test_neg_shl_sub_extra_use1(i64 %a, i64 %b, i64* %p) {
 ; CHECK-LABEL: @test_neg_shl_sub_extra_use1(
-; CHECK-NEXT:    [[SUB_NEG:%.*]] = sub i64 [[B:%.*]], [[A:%.*]]
-; CHECK-NEXT:    [[SUB:%.*]] = sub i64 [[A]], [[B]]
+; CHECK-NEXT:    [[SUB:%.*]] = sub i64 [[A:%.*]], [[B:%.*]]
 ; CHECK-NEXT:    store i64 [[SUB]], i64* [[P:%.*]], align 8
-; CHECK-NEXT:    [[MUL_NEG:%.*]] = shl i64 [[SUB_NEG]], 2
-; CHECK-NEXT:    ret i64 [[MUL_NEG]]
+; CHECK-NEXT:    [[MUL:%.*]] = shl i64 [[SUB]], 2
+; CHECK-NEXT:    [[NEG:%.*]] = sub i64 0, [[MUL]]
+; CHECK-NEXT:    ret i64 [[NEG]]
 ;
   %sub = sub i64 %a, %b
   store i64 %sub, i64* %p