fix an invisible bug when combining repeated FP divisors

[spatel]

Summary

This patch fixes bugs that were exposed by 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. If anyone sees a way to expose the bug without FMF, I'd be happy to modify the test case.

[hfinkel]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

8312 ↗

(On Diff #26173)

So once we have FMF, you'll add them here?

8324 ↗

(On Diff #26173)

I'm not sure this is the right solution. Instead, we may want to check for a pre-existing reciprocal node, and use it if it already exists instead of replacing it with, perhaps, looser FMFs.

[spatel]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

8312 ↗

(On Diff #26173)

Right - once we have FMF, this recip node should at least have 'arcp'. And that's a bug in the existing patch in D9708 ( http://reviews.llvm.org/D9708 ) because I forgot about that.

And I think any FMUL that we create below would also have FMF because it was created under an FMF precondition.

8324 ↗

(On Diff #26173)

This is where things get even foggier for me. :)
To make this optimization, don't we need to check the FMF of each U node? Eg, if one of these users does not have 'arcp', it is not eligible for replacement. I did check this in the current draft in D9708.

[spatel]

Phab test: I sent replies to Hal's comments an hour ago, but don't see anything on the commits list.

[spatel]

Adding bogus inline replies in an attempt to get the real replies that I sent yesterday to the commits list.

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

8312 ↗

(On Diff #26173)

.

8324 ↗

(On Diff #26173)

.

[spatel]

Ping.

If possible, I'd like to postpone the FMF question and just decide what the right solution for this bug is independently of FMF.
Ie, is it necessary to use CombineTo here rather than RAUW? And is it wrong to return an empty SDValue() after we changed the DAG?

[spatel]

Ping * 2.

[spatel]

It's too soon to ping again :)
but I should mention that FMF for the DAG made it back into trunk with:
http://reviews.llvm.org/D10403

The functionality is hidden behind an enable flag (disabled by default), so we can expose the bug that this patch attempts to solve by using:

$ llc -enable-fmf-dag foo.ll

If we can resolve this bug, we can move on to other bugs that FMF may have exposed.

[spatel]

Ping * 3.

[spatel]

Ping * 4.

[hfinkel]

In D9893#186998, @spatel wrote:

Ping.

If possible, I'd like to postpone the FMF question and just decide what the right solution for this bug is independently of FMF.

Okay, but please add a FIXME. Otheriwse, LGTM.

Ie, is it necessary to use CombineTo here rather than RAUW? And is it wrong to return an empty SDValue() after we changed the DAG?

[spatel]

In D9893#201564, @hfinkel wrote:

In D9893#186998, @spatel wrote:

If possible, I'd like to postpone the FMF question and just decide what the right solution for this bug is independently of FMF.

Okay, but please add a FIXME. Otheriwse, LGTM.

Thanks, Hal! Added a FIXME and checked in at r241826.

Now to ping Nick on the other thread to see what else was broken by FMF. :)

llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

if (Options.UnsafeFPMath) {
     // Find all FDIV users of the same divisor.
     for (auto *U : N1->uses()) {
       if (U->getOpcode() == ISD::FDIV && U->getOperand(1) == N1)
         Users.push_back(U);
     }
 
     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
       for (auto *U : Users) {
         SDValue Dividend = U->getOperand(0);
         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.
     }
   }
 
   return SDValue();
 }
 
 SDValue DAGCombiner::visitFREM(SDNode *N) {
   SDValue N0 = N->getOperand(0);

llvm/trunk/test/CodeGen/X86/fdiv-combine.ll

 ; CHECK-NEXT:    mulss %xmm3, %xmm0
 ; CHECK-NEXT:    retq
   %div1 = fdiv arcp float %x, %z
   %mul = fmul arcp float %div1, %y
   %div2 = fdiv arcp float %mul, %z
   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" }