Skip to content

Commit

Permalink
[SelectionDAG] Check any use of negation result before removal
Browse files Browse the repository at this point in the history
2508ef0 fixed a bug about constant removal in negation. But after
sanitizing check I found there's still some issue about it so it's
reverted.

Temporary nodes will be removed if useless in negation. Before the
removal, they'd be checked if any other nodes used it. So the removal
was moved after getNode. However in rare cases the node to be removed is
the same as result of getNode. We missed that and will be fixed by this
patch.

Reviewed By: steven.zhang

Differential Revision: https://reviews.llvm.org/D87614
  • Loading branch information
ecnelises committed Sep 17, 2020
1 parent b056292 commit a2fb544
Show file tree
Hide file tree
Showing 2 changed files with 43 additions and 7 deletions.
22 changes: 15 additions & 7 deletions llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
Expand Up @@ -5773,8 +5773,10 @@ SDValue TargetLowering::getNegatedExpression(SDValue Op, SelectionDAG &DAG,

// If we already have the use of the negated floating constant, it is free
// to negate it even it has multiple uses.
if (!Op.hasOneUse() && CFP.use_empty())
if (!Op.hasOneUse() && CFP.use_empty()) {
RemoveDeadNode(CFP);
break;
}
Cost = NegatibleCost::Neutral;
return CFP;
}
Expand Down Expand Up @@ -5832,15 +5834,17 @@ SDValue TargetLowering::getNegatedExpression(SDValue Op, SelectionDAG &DAG,
if (NegX && (CostX <= CostY)) {
Cost = CostX;
SDValue N = DAG.getNode(ISD::FSUB, DL, VT, NegX, Y, Flags);
RemoveDeadNode(NegY);
if (NegY != N)
RemoveDeadNode(NegY);
return N;
}

// Negate the Y if it is not expensive.
if (NegY) {
Cost = CostY;
SDValue N = DAG.getNode(ISD::FSUB, DL, VT, NegY, X, Flags);
RemoveDeadNode(NegX);
if (NegX != N)
RemoveDeadNode(NegX);
return N;
}
break;
Expand Down Expand Up @@ -5879,7 +5883,8 @@ SDValue TargetLowering::getNegatedExpression(SDValue Op, SelectionDAG &DAG,
if (NegX && (CostX <= CostY)) {
Cost = CostX;
SDValue N = DAG.getNode(Opcode, DL, VT, NegX, Y, Flags);
RemoveDeadNode(NegY);
if (NegY != N)
RemoveDeadNode(NegY);
return N;
}

Expand All @@ -5892,7 +5897,8 @@ SDValue TargetLowering::getNegatedExpression(SDValue Op, SelectionDAG &DAG,
if (NegY) {
Cost = CostY;
SDValue N = DAG.getNode(Opcode, DL, VT, X, NegY, Flags);
RemoveDeadNode(NegX);
if (NegX != N)
RemoveDeadNode(NegX);
return N;
}
break;
Expand Down Expand Up @@ -5923,15 +5929,17 @@ SDValue TargetLowering::getNegatedExpression(SDValue Op, SelectionDAG &DAG,
if (NegX && (CostX <= CostY)) {
Cost = std::min(CostX, CostZ);
SDValue N = DAG.getNode(Opcode, DL, VT, NegX, Y, NegZ, Flags);
RemoveDeadNode(NegY);
if (NegY != N)
RemoveDeadNode(NegY);
return N;
}

// Negate the Y if it is not expensive.
if (NegY) {
Cost = std::min(CostY, CostZ);
SDValue N = DAG.getNode(Opcode, DL, VT, X, NegY, NegZ, Flags);
RemoveDeadNode(NegX);
if (NegX != N)
RemoveDeadNode(NegX);
return N;
}
break;
Expand Down
28 changes: 28 additions & 0 deletions llvm/test/CodeGen/X86/pr47517.ll
@@ -0,0 +1,28 @@
; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
; RUN: llc -mtriple x86_64 < %s | FileCheck %s

; To ensure unused floating point constant is correctly removed
define float @test(float %src, float* %p) {
; CHECK-LABEL: test:
; CHECK: # %bb.0: # %entry
; CHECK-NEXT: movq $0, (%rdi)
; CHECK-NEXT: xorps %xmm0, %xmm0
; CHECK-NEXT: retq
entry:
%a0 = getelementptr inbounds float, float* %p, i32 0
%a1 = getelementptr inbounds float, float* %p, i32 1
store float 0.000000e+00, float* %a0
store float 0.000000e+00, float* %a1
%zero = load float, float* %a0
%fmul1 = fmul fast float %zero, %src
%fadd1 = fadd fast float %fmul1, %zero
%fmul2 = fmul fast float %fadd1, 2.000000e+00
%fmul3 = fmul fast float %fmul2, %fmul2
%fmul4 = fmul fast float %fmul2, 2.000000e+00
%fadd2 = fadd fast float %fmul4, -3.000000e+00
%fmul5 = fmul fast float %fadd2, %fmul2
%fadd3 = fadd fast float %fmul2, %src
%fadd4 = fadd fast float %fadd3, %fmul5
%fmul6 = fmul fast float %fmul3, %fadd4
ret float %fmul6
}

1 comment on commit a2fb544

@ecnelises
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The differential revision should be https://reviews.llvm.org/D87698

Please sign in to comment.