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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm created this revision.
arsenm added reviewers: RKSimon, spatel, mcberg2017, cameron.mcinally, uweigand.
Herald added subscribers: hiraditya, tpr, nhaehnle, wdng, jvesely.
Herald added a project: LLVM.

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.


https://reviews.llvm.org/D72015

Files:
  llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
  llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
  llvm/test/CodeGen/AMDGPU/fneg-fold-legalize-dag-increase-insts.ll


Index: llvm/test/CodeGen/AMDGPU/fneg-fold-legalize-dag-increase-insts.ll
===================================================================
--- /dev/null
+++ llvm/test/CodeGen/AMDGPU/fneg-fold-legalize-dag-increase-insts.ll
@@ -0,0 +1,24 @@
+; 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
+; 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[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
+}
Index: llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
===================================================================
--- llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
+++ llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
@@ -4688,11 +4688,6 @@
     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;
Index: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
===================================================================
--- llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
+++ llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
@@ -13350,6 +13350,16 @@
   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) &&


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D72015.235643.patch
Type: text/x-patch
Size: 3107 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20191230/4c6460e5/attachment.bin>