[DAGCombine] Don't delete the node if it has uses immediately

[steven.zhang]

Summary

This is the follow up patch for https://reviews.llvm.org/D86183 as we miss to delete the node if NegX == NegY, which has use after we create the node.

if (NegX && (CostX <= CostY)) {
  Cost = std::min(CostX, CostZ);
  RemoveDeadNode(NegY);
  return DAG.getNode(Opcode, DL, VT, NegX, Y, NegZ, Flags);  #<-- NegY is used here if NegY == NegX.
}

[spatel]

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp

5823

I think it would be clearer and less code duplication to put the 'if' check into the lambda, so something like:

auto RemoveDeadNode = [&](SDValue N, SDValue Other) {
  if (N && N.getNode()->use_empty() && N != Other)
    DAG.RemoveDeadNode(N.getNode());
};

Then call it with something like this:

RemoveDeadNode(NegX, NegY);

llvm/test/CodeGen/PowerPC/fneg.ll

56–62

We should reduce the test to avoid UB and extra operations, so something like this should work?

define double @fneg_no_ice(float %x) {
  %y = fsub fast float 1.0, %x
  %e = fpext float %y to double
  %e2 = fmul double %e, %e
  %e3 = fmul double %e, %e2
  ret double %e3
}

[steven.zhang]

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp

5823

we need also check NegX != NegZ...

[steven.zhang]

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp

5823

So, I think, move it after the getNode() will be more safe and make RemoveDeadNode() semantics clean. Otherwise, we have to check NegY != NegX and NegY != NegZ for FMA.

llvm/test/CodeGen/PowerPC/fneg.ll

56–62

Good point. Yes, it works.

[steven.zhang]

Update test.

[spatel]

LGTM

[jsji]

Commit on behalf of @steven.zhang to unblock internal build first..

[hans]

Cherry-picked to 11.x as defbc77a7c951d8cc47daf49df15c7f548cada05.

[uabelho]

Hi,

I wrote https://bugs.llvm.org/show_bug.cgi?id=47517 about a crash that starts happening with this patch.

[steven.zhang]

https://reviews.llvm.org/D87614 is posted to fix this issue.

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp

case ISD::FADD: {
     // fold (fneg (fadd X, Y)) -> (fsub (fneg Y), X)
     NegatibleCost CostY = NegatibleCost::Expensive;
     SDValue NegY =
         getNegatedExpression(Y, DAG, LegalOps, OptForSize, CostY, Depth);
 
     // Negate the X if its cost is less or equal than Y.
     if (NegX && (CostX <= CostY)) {
       Cost = CostX;
+      SDValue N = DAG.getNode(ISD::FSUB, DL, VT, NegX, Y, Flags);
       RemoveDeadNode(NegY);
-      return DAG.getNode(ISD::FSUB, DL, VT, NegX, Y, Flags);
+      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);
-      return DAG.getNode(ISD::FSUB, DL, VT, NegY, X, Flags);
+      return N;
     }
     break;
   }
   case ISD::FSUB: {
     // We can't turn -(A-B) into B-A when we honor signed zeros.
     if (!Options.NoSignedZerosFPMath && !Flags.hasNoSignedZeros())
       break;
 
case ISD::FDIV: {
     // fold (fneg (fmul X, Y)) -> (fmul X, (fneg Y))
     NegatibleCost CostY = NegatibleCost::Expensive;
     SDValue NegY =
         getNegatedExpression(Y, DAG, LegalOps, OptForSize, CostY, Depth);
 
     // Negate the X if its cost is less or equal than Y.
     if (NegX && (CostX <= CostY)) {
       Cost = CostX;
+      SDValue N = DAG.getNode(Opcode, DL, VT, NegX, Y, Flags);
       RemoveDeadNode(NegY);
-      return DAG.getNode(Opcode, DL, VT, NegX, Y, Flags);
+      return N;
     }
 
     // Ignore X * 2.0 because that is expected to be canonicalized to X + X.
     if (auto *C = isConstOrConstSplatFP(Op.getOperand(1)))
       if (C->isExactlyValue(2.0) && Op.getOpcode() == ISD::FMUL)
         break;
 
     // Negate the Y if it is not expensive.
     if (NegY) {
       Cost = CostY;
+      SDValue N = DAG.getNode(Opcode, DL, VT, X, NegY, Flags);
       RemoveDeadNode(NegX);
-      return DAG.getNode(Opcode, DL, VT, X, NegY, Flags);
+      return N;
     }
     break;
   }
   case ISD::FMA:
   case ISD::FMAD: {
     if (!Options.NoSignedZerosFPMath && !Flags.hasNoSignedZeros())
       break;
 
case ISD::FMAD: {
     // fold (fneg (fma X, Y, Z)) -> (fma X, (fneg Y), (fneg Z))
     NegatibleCost CostY = NegatibleCost::Expensive;
     SDValue NegY =
         getNegatedExpression(Y, DAG, LegalOps, OptForSize, CostY, Depth);
 
     // Negate the X if its cost is less or equal than Y.
     if (NegX && (CostX <= CostY)) {
       Cost = std::min(CostX, CostZ);
+      SDValue N = DAG.getNode(Opcode, DL, VT, NegX, Y, NegZ, Flags);
       RemoveDeadNode(NegY);
-      return DAG.getNode(Opcode, DL, VT, NegX, Y, NegZ, Flags);
+      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);
-      return DAG.getNode(Opcode, DL, VT, X, NegY, NegZ, Flags);
+      return N;
     }
     break;
   }
 
   case ISD::FP_EXTEND:
   case ISD::FSIN:
     if (SDValue NegV = getNegatedExpression(Op.getOperand(0), DAG, LegalOps,
                                             OptForSize, Cost, Depth))

llvm/test/CodeGen/PowerPC/fneg.ll

 ; CHECK-NEXT:    fsubs f0, f3, f2
 ; CHECK-NEXT:    fmadds f1, f1, f0, f4
 ; CHECK-NEXT:    blr
   %negx = fneg float %x
   %negy = fsub nsz float %y0, %y1
   %r = call float @llvm.fmuladd.f32(float %negx, float %negy, float %z)
   ret float %r
 }
+
+; Verify that we didn't hit assertion for this case.
+define double @fneg_no_ice(float %x) {
+; CHECK-LABEL: fneg_no_ice:
+; CHECK:       # %bb.0:
+; CHECK-NEXT:    lis r3, .LCPI3_0@ha
+; CHECK-NEXT:    lfs f0, .LCPI3_0@l(r3)
+; CHECK-NEXT:    fsubs f0, f0, f1
+; CHECK-NEXT:    fmul f1, f0, f0
+; CHECK-NEXT:    fmul f1, f0, f1
+; CHECK-NEXT:    blr
+  %y = fsub fast float 1.0, %x
+  %e = fpext float %y to double
+  %e2 = fmul double %e, %e
+  %e3 = fmul double %e, %e2
+  ret double %e3
+}