DAG: Stop trying to fold FP -(x-y) -> y-x in getNode with nsz

[arsenm]

Summary

This was increasing the number of instructions when fsub was legalized
on AMDGPU with no signed zeros enabled. This fold should be guarded by
hasOneUse, and I don't think getNode should be doing that. The same
fold is already done as a regular combine through isNegatibleForFree.

This does require duplicating, even though isNegatibleForFree does
this combine already (and properly checks hasOneUse) to avoid one PPC
regression. In the regression, the outer fneg has nsz but the fsub
operand does not. isNegatibleForFree only sees the operand, and
doesn't see it's used from a nsz context. A nsz parameter needs to be
added and threaded through isNegatibleForFree to avoid this.

[spatel]

The transform doesn't necessarily add an instruction (depends on the target), but I agree that we don't want to replace an fneg generically with an fsub - even if that reduces the dependency chain, the fneg might be cheap/free relative to an independent FP op. I've added an AArch64 test here that should wiggle with this patch:
rGe6bdecf1cd6b

Please rebase (and would be better to have the baseline AMDGPU test as a preliminary commit too).

[arsenm]

Rebase. New AArch64 test looks like an improvement

[spatel]

LGTM

[arsenm]

4d7201e7b988b62a6ca30416fd03847b5a39dae0

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

SDValue DAGCombiner::visitFNEG(SDNode *N) {
 
   // Constant fold FNEG.
   if (isConstantFPBuildVectorOrConstantFP(N0))
     return DAG.getNode(ISD::FNEG, SDLoc(N), VT, N0);
 
   if (TLI.isNegatibleForFree(N0, DAG, LegalOperations, ForCodeSize))
     return TLI.getNegatedExpression(N0, DAG, LegalOperations, ForCodeSize);
 
+  // -(X-Y) -> (Y-X) is unsafe because when X==Y, -0.0 != +0.0 FIXME: This is
+  // duplicated in isNegatibleForFree, but isNegatibleForFree doesn't know it
+  // was called from a context with a nsz flag if the input fsub does not.
+  if (N0.getOpcode() == ISD::FSUB &&
+      (DAG.getTarget().Options.NoSignedZerosFPMath ||
+       N->getFlags().hasNoSignedZeros()) && N0.hasOneUse()) {
+    return DAG.getNode(ISD::FSUB, SDLoc(N), VT, N0.getOperand(1),
+                       N0.getOperand(0), N->getFlags());
+  }
+
   // Transform fneg(bitconvert(x)) -> bitconvert(x ^ sign) to avoid loading
   // constant pool values.
   if (!TLI.isFNegFree(VT) &&
       N0.getOpcode() == ISD::BITCAST &&
       N0.getNode()->hasOneUse()) {
     SDValue Int = N0.getOperand(0);
     EVT IntVT = Int.getValueType();
     if (IntVT.isInteger() && !IntVT.isVector()) {

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp

if (OpOpcode == ISD::EXTRACT_VECTOR_ELT &&
         Operand.getOperand(0).getValueType() == VT)
       return Operand.getOperand(0);
     break;
   case ISD::FNEG:
     // Negation of an unknown bag of bits is still completely undefined.
     if (OpOpcode == ISD::UNDEF)
       return getUNDEF(VT);
 
-    // -(X-Y) -> (Y-X) is unsafe because when X==Y, -0.0 != +0.0
-    if ((getTarget().Options.NoSignedZerosFPMath || Flags.hasNoSignedZeros()) &&
-        OpOpcode == ISD::FSUB)
-      return getNode(ISD::FSUB, DL, VT, Operand.getOperand(1),
-                     Operand.getOperand(0), Flags);
     if (OpOpcode == ISD::FNEG)  // --X -> X
       return Operand.getOperand(0);
     break;
   case ISD::FABS:
     if (OpOpcode == ISD::FNEG)  // abs(-X) -> abs(X)
       return getNode(ISD::FABS, DL, VT, Operand.getOperand(0));
     break;
   }

llvm/test/CodeGen/AArch64/arm64-fp.ll

; CHECK-NEXT:    ret
   %t16 = fmul double %t, %t15
   %t18 = fadd double %t16, %t7
   ret double %t18
 }
 
 define { double, double } @testfn(double %x, double %y) #0 {
 ; CHECK-LABEL: testfn:
 ; CHECK:       // %bb.0:
-; CHECK-NEXT:    fsub d2, d0, d1
-; CHECK-NEXT:    fsub d1, d1, d0
-; CHECK-NEXT:    mov v0.16b, v2.16b
+; CHECK-NEXT:    fsub d0, d0, d1
+; CHECK-NEXT:    fneg d1, d0
 ; CHECK-NEXT:    ret
   %sub = fsub fast double %x, %y
   %neg = fneg fast double %sub
   %r0 = insertvalue { double, double } undef, double %sub, 0
   %r1 = insertvalue { double, double } %r0, double %neg, 1
   ret { double, double } %r1
 }
 
 attributes #0 = { "no-signed-zeros-fp-math"="true" }

llvm/test/CodeGen/AMDGPU/fneg-fold-legalize-dag-increase-insts.ll

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc -mtriple=amdgcn-amd-amdhsa -mcpu=gfx900 -enable-no-signed-zeros-fp-math=true < %s | FileCheck %s
-
-; FIXME: This should be the same when the extra instructions are fixed
-; XUN: llc -mtriple=amdgcn-amd-amdhsa -mcpu=gfx900 -enable-no-signed-zeros-fp-math=false < %s | FileCheck %s
+; RUN: llc -mtriple=amdgcn-amd-amdhsa -mcpu=gfx900 -enable-no-signed-zeros-fp-math=false < %s | FileCheck %s
 
 ; no-signed-zeros-fp-math should not increase the number of
 ; instructions emitted.
 
 define { double, double } @testfn(double %arg, double %arg1, double %arg2) {
 ; CHECK-LABEL: testfn:
 ; CHECK:       ; %bb.0: ; %bb
 ; CHECK-NEXT:    s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
-; CHECK-NEXT:    v_add_f64 v[6:7], v[4:5], -v[0:1]
-; CHECK-NEXT:    v_add_f64 v[4:5], v[0:1], -v[4:5]
-; CHECK-NEXT:    v_add_f64 v[0:1], v[6:7], -v[2:3]
-; CHECK-NEXT:    v_add_f64 v[2:3], -v[2:3], v[4:5]
+; CHECK-NEXT:    v_add_f64 v[4:5], v[4:5], -v[0:1]
+; CHECK-NEXT:    v_add_f64 v[0:1], v[4:5], -v[2:3]
+; CHECK-NEXT:    v_add_f64 v[2:3], -v[2:3], -v[4:5]
 ; CHECK-NEXT:    s_setpc_b64 s[30:31]
 bb:
   %tmp = fsub fast double 0.000000e+00, %arg1
   %tmp3 = fsub fast double %arg2, %arg
   %tmp4 = fadd fast double %tmp3, %tmp
   %tmp5 = fsub fast double %tmp, %tmp3
   %tmp6 = insertvalue { double, double } undef, double %tmp4, 0
   %tmp7 = insertvalue { double, double } %tmp6, double %tmp5, 1
   ret { double, double } %tmp7
 }