[llvm] r241826 - fix an invisible bug when combining repeated FP divisors

[Sanjay Patel]

Author: spatel
Date: Thu Jul  9 12:28:37 2015
New Revision: 241826

URL: http://llvm.org/viewvc/llvm-project?rev=241826&view=rev
Log:
fix an invisible bug when combining repeated FP divisors

This patch fixes bugs that were exposed by the addition of fast-math-flags in the DAG:
r237046 ( http://reviews.llvm.org/rL237046 ):

1. When replacing a division node, it's not enough to RAUW.
   We should call CombineTo() to delete dead nodes and combine again.
2. Because we are changing the DAG, we can't return an empty SDValue
   after the transform. As the code comments say:

    Visitation implementation - Implement dag node combining for different node types.
    The semantics are as follows: Return Value:
      SDValue.getNode() == 0 - No change was made
      SDValue.getNode() == N - N was replaced, is dead and has been handled.
      otherwise - N should be replaced by the returned Operand.

The new test case shows no difference with or without this patch, but it will crash if
we re-apply r237046 or enable FMF via the current -enable-fmf-dag cl::opt.

Differential Revision: http://reviews.llvm.org/D9893


Modified:
    llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
    llvm/trunk/test/CodeGen/X86/fdiv-combine.ll

Modified: llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp?rev=241826&r1=241825&r2=241826&view=diff
==============================================================================
--- llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp (original)
+++ llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp Thu Jul  9 12:28:37 2015
@@ -8374,6 +8374,9 @@ SDValue DAGCombiner::visitFDIV(SDNode *N
 
     if (TLI.combineRepeatedFPDivisors(Users.size())) {
       SDValue FPOne = DAG.getConstantFP(1.0, DL, VT);
+      // FIXME: This optimization requires some level of fast-math, so the
+      // created reciprocal node should at least have the 'allowReciprocal'
+      // fast-math-flag set.
       SDValue Reciprocal = DAG.getNode(ISD::FDIV, DL, VT, FPOne, N1);
 
       // Dividend / Divisor -> Dividend * Reciprocal
@@ -8382,10 +8385,14 @@ SDValue DAGCombiner::visitFDIV(SDNode *N
         if (Dividend != FPOne) {
           SDValue NewNode = DAG.getNode(ISD::FMUL, SDLoc(U), VT, Dividend,
                                         Reciprocal);
-          DAG.ReplaceAllUsesWith(U, NewNode.getNode());
+          CombineTo(U, NewNode);
+        } else if (U != Reciprocal.getNode()) {
+          // In the absence of fast-math-flags, this user node is always the
+          // same node as Reciprocal, but with FMF they may be different nodes.
+          CombineTo(U, Reciprocal);
         }
       }
-      return SDValue();
+      return SDValue(N, 0);  // N was replaced.
     }
   }
 

Modified: llvm/trunk/test/CodeGen/X86/fdiv-combine.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/X86/fdiv-combine.ll?rev=241826&r1=241825&r2=241826&view=diff
==============================================================================
--- llvm/trunk/test/CodeGen/X86/fdiv-combine.ll (original)
+++ llvm/trunk/test/CodeGen/X86/fdiv-combine.ll Thu Jul  9 12:28:37 2015
@@ -27,5 +27,22 @@ define float @div2_arcp(float %x, float
   ret float %div2
 }
 
+; If the reciprocal is already calculated, we should not
+; generate an extra multiplication by 1.0. 
+
+define double @div3_arcp(double %x, double %y, double %z) #0 {
+; CHECK-LABEL: div3_arcp:
+; CHECK:       # BB#0:
+; CHECK-NEXT:    movsd{{.*#+}} xmm2 = mem[0],zero
+; CHECK-NEXT:    divsd %xmm1, %xmm2
+; CHECK-NEXT:    mulsd %xmm2, %xmm0
+; CHECK-NEXT:    addsd %xmm2, %xmm0
+; CHECK-NEXT:    retq
+  %div1 = fdiv fast double 1.0, %y
+  %div2 = fdiv fast double %x, %y
+  %ret = fadd fast double %div2, %div1
+  ret double %ret
+}
+
 ; FIXME: If the backend understands 'arcp', then this attribute is unnecessary.
 attributes #0 = { "unsafe-fp-math"="true" }