[llvm] 5a159ed - [InstCombine] Negator: don't negate multi-use `sub`
Kostya Serebryany via llvm-commits
llvm-commits at lists.llvm.org
Thu Apr 23 16:53:02 PDT 2020
thanks a lot for the fix!
On Thu, Apr 23, 2020 at 2:01 PM Roman Lebedev via llvm-commits <
llvm-commits at lists.llvm.org> wrote:
>
> 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
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200423/be02ed38/attachment.html>
More information about the llvm-commits
mailing list