[llvm] 5a159ed - [InstCombine] Negator: don't negate multi-use `sub`
Roman Lebedev via llvm-commits
llvm-commits at lists.llvm.org
Thu Apr 23 14:00:54 PDT 2020
Author: Roman Lebedev
Date: 2020-04-23T23:59:15+03:00
New Revision: 5a159ed2a8e5a9a6ced73f78e4c64b01d76d3493
URL: https://github.com/llvm/llvm-project/commit/5a159ed2a8e5a9a6ced73f78e4c64b01d76d3493
DIFF: https://github.com/llvm/llvm-project/commit/5a159ed2a8e5a9a6ced73f78e4c64b01d76d3493.diff
LOG: [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.
Added:
Modified:
llvm/lib/Transforms/InstCombine/InstCombineNegator.cpp
llvm/test/Transforms/InstCombine/sub-of-negatible.ll
llvm/test/Transforms/InstCombine/sub.ll
Removed:
################################################################################
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineNegator.cpp b/llvm/lib/Transforms/InstCombine/InstCombineNegator.cpp
index e119a383e9db..42bb748cc287 100644
--- 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 83f86ca7a5e1..54896397f5cf 100644
--- 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 1b4149291951..f6fa797eb0c8 100644
--- 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
More information about the llvm-commits
mailing list