Use the 'arcp' fast-math-flag when combining repeated FP divisors

[spatel]

Summary

This is a usage of the IR-level fast-math-flags now that they are propagated to SDNodes (r237046). This was originally part of D8900.

I'm not sure if we need to maintain the global TargetOptions.UnsafeFPMath checks or if we're now free to start deleting those and rely exclusively on the node-level optimization flags. If the latter, I need to update the corresponding fdiv testcases for PowerPC and Aarch64 to use 'arcp'.

[ahatanak]

To preserve backward compatibility, I think we should make sure functions using fast math attributes (e.g., "unsafe-fp-math" in test/CodeGen/PowerPC/fdiv-combine.ll) still get optimized after we migrate to modeling those flags at the node-level (using autoupgrade?).

[spatel]

In D9708#171355, @ahatanak wrote:

To preserve backward compatibility, I think we should make sure functions using fast math attributes (e.g., "unsafe-fp-math" in test/CodeGen/PowerPC/fdiv-combine.ll) still get optimized after we migrate to modeling those flags at the node-level (using autoupgrade?).

Thanks, Akira. If I understand correctly, I should create this autoupgrade patch first then. That would allow us to remove the Options.UnsafeFPMath checks as we update all of the places in the backend to use the node-level flags.

Is a separate mechanism needed to handle the llc command-line flag "-enable-unsafe-fp-math"?

[spatel]

Sorry for the (5 month!) delay...

After several false starts, FMF on DAG nodes appears to be here for good. This will still be the first use of them to enable an optimization from what I can tell.

Patch rebased.

[hfinkel]

This will still be the first use of them to enable an optimization from what I can tell.

:-)

LGTM.

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

 // Combine multiple FDIVs with the same divisor into multiple FMULs by the
 // reciprocal.
 // E.g., (a / D; b / D;) -> (recip = 1.0 / D; a * recip; b * recip)
 // Notice that this is not always beneficial. One reason is different target
 // may have different costs for FDIV and FMUL, so sometimes the cost of two
 // FDIVs may be lower than the cost of one FDIV and two FMULs. Another reason
 // is the critical path is increased from "one FDIV" to "one FDIV + one FMUL".
 SDValue DAGCombiner::combineRepeatedFPDivisors(SDNode *N) {
-  if (!DAG.getTarget().Options.UnsafeFPMath)
+  bool UnsafeMath = DAG.getTarget().Options.UnsafeFPMath;
+  const SDNodeFlags *Flags = N->getFlags();
+  if (!UnsafeMath && !Flags->hasAllowReciprocal())
     return SDValue();
 
   // Skip if current node is a reciprocal.
   SDValue N0 = N->getOperand(0);
   ConstantFPSDNode *N0CFP = dyn_cast<ConstantFPSDNode>(N0);
   if (N0CFP && N0CFP->isExactlyValue(1.0))
     return SDValue();
 
   // Exit early if the target does not want this transform or if there can't
   // possibly be enough uses of the divisor to make the transform worthwhile.
   SDValue N1 = N->getOperand(1);
   unsigned MinUses = TLI.combineRepeatedFPDivisors();
   if (!MinUses || N1->use_size() < MinUses)
     return SDValue();
 
   // Find all FDIV users of the same divisor.
   // Use a set because duplicates may be present in the user list.
   SetVector<SDNode *> Users;
-  for (auto *U : N1->uses())
-    if (U->getOpcode() == ISD::FDIV && U->getOperand(1) == N1)
+  for (auto *U : N1->uses()) {
+    if (U->getOpcode() == ISD::FDIV && U->getOperand(1) == N1) {
+      // This division is eligible for optimization only if global unsafe math
+      // is enabled or if this division allows reciprocal formation.
+      if (UnsafeMath || U->getFlags()->hasAllowReciprocal())
         Users.insert(U);
+    }
+  }
 
   // Now that we have the actual number of divisor uses, make sure it meets
   // the minimum threshold specified by the target.
   if (Users.size() < MinUses)
     return SDValue();
 
   EVT VT = N->getValueType(0);
   SDLoc DL(N);
   SDValue FPOne = DAG.getConstantFP(1.0, DL, VT);
-  const SDNodeFlags *Flags = &cast<BinaryWithFlagsSDNode>(N)->Flags;
   SDValue Reciprocal = DAG.getNode(ISD::FDIV, DL, VT, FPOne, N1, Flags);
 
   // 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, Flags);

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

 ; RUN: llc < %s -mtriple=x86_64-unknown-unknown -mcpu=x86-64 | FileCheck %s
 
-; Anything more than one division using a single divisor operand
+; More than one 'arcp' division using a single divisor operand
 ; should be converted into a reciprocal and multiplication.
 
-define float @div1_arcp(float %x, float %y, float %z) #0 {
+; Don't do anything for just one division.
+
+define float @div1_arcp(float %x, float %y, float %z) {
 ; CHECK-LABEL: div1_arcp:
 ; CHECK:       # BB#0:
 ; CHECK-NEXT:    divss %xmm1, %xmm0
 ; CHECK-NEXT:    retq
   %div1 = fdiv arcp float %x, %y
   ret float %div1
 }
 
-define float @div2_arcp(float %x, float %y, float %z) #0 {
-; CHECK-LABEL: div2_arcp:
+; All math instructions are 'arcp', so optimize.
+
+define float @div2_arcp_all(float %x, float %y, float %z) {
+; CHECK-LABEL: div2_arcp_all:
 ; CHECK:       # BB#0:
 ; CHECK-NEXT:    movss {{.*#+}} xmm3 = mem[0],zero,zero,zero
 ; CHECK-NEXT:    divss %xmm2, %xmm3
-; CHECK-NEXT:    mulss %xmm1, %xmm0
 ; CHECK-NEXT:    mulss %xmm3, %xmm0
+; CHECK-NEXT:    mulss %xmm1, %xmm0
 ; 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
 }
 
+; The first division is not 'arcp', so do not optimize.
+
+define float @div2_arcp_partial1(float %x, float %y, float %z) {
+; CHECK-LABEL: div2_arcp_partial1:
+; CHECK:       # BB#0:
+; CHECK-NEXT:    divss %xmm2, %xmm0
+; CHECK-NEXT:    mulss %xmm1, %xmm0
+; CHECK-NEXT:    divss %xmm2, %xmm0
+; CHECK-NEXT:    retq
+  %div1 = fdiv float %x, %z
+  %mul = fmul arcp float %div1, %y
+  %div2 = fdiv arcp float %mul, %z
+  ret float %div2
+}
+
+; The second division is not 'arcp', so do not optimize.
+
+define float @div2_arcp_partial2(float %x, float %y, float %z) {
+; CHECK-LABEL: div2_arcp_partial2:
+; CHECK:       # BB#0:
+; CHECK-NEXT:    divss %xmm2, %xmm0
+; CHECK-NEXT:    mulss %xmm1, %xmm0
+; CHECK-NEXT:    divss %xmm2, %xmm0
+; CHECK-NEXT:    retq
+  %div1 = fdiv arcp float %x, %z
+  %mul = fmul arcp float %div1, %y
+  %div2 = fdiv float %mul, %z
+  ret float %div2
+}
+
+; The multiply is not 'arcp', but that does not prevent optimizing the divisions.
+
+define float @div2_arcp_partial3(float %x, float %y, float %z) {
+; CHECK-LABEL: div2_arcp_partial3:
+; CHECK:       # BB#0:
+; CHECK-NEXT:    movss {{.*#+}} xmm3 = mem[0],zero,zero,zero
+; CHECK-NEXT:    divss %xmm2, %xmm3
+; CHECK-NEXT:    mulss %xmm3, %xmm0
+; CHECK-NEXT:    mulss %xmm1, %xmm0
+; CHECK-NEXT:    mulss %xmm3, %xmm0
+; CHECK-NEXT:    retq
+  %div1 = fdiv arcp float %x, %z
+  %mul = fmul 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 {
+define double @div3_arcp(double %x, double %y, double %z) {
 ; 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
 }
 
-define void @PR24141() #0 {
+define void @PR24141() {
 ; CHECK-LABEL: PR24141:
 ; CHECK:  callq
 ; CHECK-NEXT: divsd
 ; CHECK-NEXT: jmp
 entry:
   br label %while.body
 
 while.body:
   %x.0 = phi double [ undef, %entry ], [ %div, %while.body ]
   %call = call { double, double } @g(double %x.0)
   %xv0 = extractvalue { double, double } %call, 0
   %xv1 = extractvalue { double, double } %call, 1
-  %div = fdiv double %xv0, %xv1
+  %div = fdiv arcp double %xv0, %xv1
   br label %while.body
 }
 
 declare { double, double } @g(double)
 
-; FIXME: If the backend understands 'arcp', then this attribute is unnecessary.
-attributes #0 = { "unsafe-fp-math"="true" }