[DAGCombiner] Remove explicit call to AddToWorklist in sqrt and reciprocal computations

[deadalnix]

Summary

These nodes end up being processed regardless due to DAGCombiner ensuring arguments are processed. This changes the order in which nodes are processed, which fixes an issue on PowerPC.

[RKSimon]

test/CodeGen/PowerPC/qpx-recipest.ll

205 ↗

(On Diff #216456)

Is the qvfcmpeq removed as well?

[deadalnix]

test/CodeGen/PowerPC/qpx-recipest.ll

205 ↗

(On Diff #216456)

Let me double check, it may be an error of mine.

[RKSimon]

test/CodeGen/PowerPC/qpx-recipest.ll

205 ↗

(On Diff #216456)

How bad would it be to use the update script and regenerate all the CHECKs?

[deadalnix]

Restore check for qvfcmpeq

[RKSimon]

LGTM - cheers

[deadalnix]

test/CodeGen/PowerPC/qpx-recipest.ll

205 ↗

(On Diff #216456)

As far as I know, this is not available for PowerPC, unless something changed recently.

[lebedev.ri]

test/CodeGen/PowerPC/qpx-recipest.ll

205 ↗

(On Diff #216456)

Shouldn't be too complicated to add

[jsji]

test/CodeGen/PowerPC/qpx-recipest.ll

205 ↗

(On Diff #216456)

I just tried, it should work.

[RKSimon]

Updated in rL369659

[deadalnix]

test/CodeGen/PowerPC/qpx-recipest.ll

205 ↗

(On Diff #216456)

As it turns out, I'm incorrect, so I'll do this and update the diff accordingly before landing.

[xbolva00]

test/CodeGen/PowerPC/qpx-recipest.ll

205 ↗

(On Diff #216456)

./update_llc_test_checks.py ../test/CodeGen/PowerPC/qpx-recipest.ll

./llvm-lit /home/xbolva00/LLVM/llvm/test/CodeGen/PowerPC/qpx-recipest.ll

• Testing: 1 tests, single process --

PASS: LLVM :: CodeGen/PowerPC/qpx-recipest.ll (1 of 1)
Testing Time: 3.74s

Expected Passes    : 1

[jsji]

test/CodeGen/PowerPC/qpx-recipest.ll

205 ↗

(On Diff #216456)

Simon have commited it in https://reviews.llvm.org/rL369659

[deadalnix]

rebase and regenerate test

[RKSimon]

cheers @deadalnix

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

if (SDValue Est = TLI.getRecipEstimate(Op, DAG, Enabled, Iterations)) {
 
     if (Iterations) {
       SDLoc DL(Op);
       SDValue FPOne = DAG.getConstantFP(1.0, DL, VT);
 
       // Newton iterations: Est = Est + Est (1 - Arg * Est)
       for (int i = 0; i < Iterations; ++i) {
         SDValue NewEst = DAG.getNode(ISD::FMUL, DL, VT, Op, Est, Flags);
-        AddToWorklist(NewEst.getNode());
-
         NewEst = DAG.getNode(ISD::FSUB, DL, VT, FPOne, NewEst, Flags);
-        AddToWorklist(NewEst.getNode());
-
         NewEst = DAG.getNode(ISD::FMUL, DL, VT, Est, NewEst, Flags);
-        AddToWorklist(NewEst.getNode());
-
         Est = DAG.getNode(ISD::FADD, DL, VT, Est, NewEst, Flags);
-        AddToWorklist(Est.getNode());
       }
     }
     return Est;
   }
 
   return SDValue();
 }
 
 /// Newton iteration for a function: F(X) is X_{i+1} = X_i - F(X_i)/F'(X_i)
 /// For the reciprocal sqrt, we need to find the zero of the function:
 ///   F(X) = 1/X^2 - A [which has a zero at X = 1/sqrt(A)]
 ///     =>
 ///   X_{i+1} = X_i (1.5 - A X_i^2 / 2)
 /// As a result, we precompute A/2 prior to the iteration loop.
 SDValue DAGCombiner::buildSqrtNROneConst(SDValue Arg, SDValue Est,
                                          unsigned Iterations,
                                          SDNodeFlags Flags, bool Reciprocal) {
   EVT VT = Arg.getValueType();
   SDLoc DL(Arg);
   SDValue ThreeHalves = DAG.getConstantFP(1.5, DL, VT);
 
   // We now need 0.5 * Arg which we can write as (1.5 * Arg - Arg) so that
   // this entire sequence requires only one FP constant.
   SDValue HalfArg = DAG.getNode(ISD::FMUL, DL, VT, ThreeHalves, Arg, Flags);
-  AddToWorklist(HalfArg.getNode());
-
   HalfArg = DAG.getNode(ISD::FSUB, DL, VT, HalfArg, Arg, Flags);
-  AddToWorklist(HalfArg.getNode());
 
   // Newton iterations: Est = Est * (1.5 - HalfArg * Est * Est)
   for (unsigned i = 0; i < Iterations; ++i) {
     SDValue NewEst = DAG.getNode(ISD::FMUL, DL, VT, Est, Est, Flags);
-    AddToWorklist(NewEst.getNode());
-
     NewEst = DAG.getNode(ISD::FMUL, DL, VT, HalfArg, NewEst, Flags);
-    AddToWorklist(NewEst.getNode());
-
     NewEst = DAG.getNode(ISD::FSUB, DL, VT, ThreeHalves, NewEst, Flags);
-    AddToWorklist(NewEst.getNode());
-
     Est = DAG.getNode(ISD::FMUL, DL, VT, Est, NewEst, Flags);
-    AddToWorklist(Est.getNode());
   }
 
   // If non-reciprocal square root is requested, multiply the result by Arg.
-  if (!Reciprocal) {
+  if (!Reciprocal)
     Est = DAG.getNode(ISD::FMUL, DL, VT, Est, Arg, Flags);
-    AddToWorklist(Est.getNode());
-  }
 
   return Est;
 }
 
 /// Newton iteration for a function: F(X) is X_{i+1} = X_i - F(X_i)/F'(X_i)
 /// For the reciprocal sqrt, we need to find the zero of the function:
 ///   F(X) = 1/X^2 - A [which has a zero at X = 1/sqrt(A)]
 ///     =>
SDValue DAGCombiner::buildSqrtNRTwoConst(SDValue Arg, SDValue Est,
   // This routine must enter the loop below to work correctly
   // when (Reciprocal == false).
   assert(Iterations > 0);
 
   // Newton iterations for reciprocal square root:
   // E = (E * -0.5) * ((A * E) * E + -3.0)
   for (unsigned i = 0; i < Iterations; ++i) {
     SDValue AE = DAG.getNode(ISD::FMUL, DL, VT, Arg, Est, Flags);
-    AddToWorklist(AE.getNode());
-
     SDValue AEE = DAG.getNode(ISD::FMUL, DL, VT, AE, Est, Flags);
-    AddToWorklist(AEE.getNode());
-
     SDValue RHS = DAG.getNode(ISD::FADD, DL, VT, AEE, MinusThree, Flags);
-    AddToWorklist(RHS.getNode());
 
     // When calculating a square root at the last iteration build:
     // S = ((A * E) * -0.5) * ((A * E) * E + -3.0)
     // (notice a common subexpression)
     SDValue LHS;
     if (Reciprocal || (i + 1) < Iterations) {
       // RSQRT: LHS = (E * -0.5)
       LHS = DAG.getNode(ISD::FMUL, DL, VT, Est, MinusHalf, Flags);
     } else {
       // SQRT: LHS = (A * E) * -0.5
       LHS = DAG.getNode(ISD::FMUL, DL, VT, AE, MinusHalf, Flags);
     }
-    AddToWorklist(LHS.getNode());
 
     Est = DAG.getNode(ISD::FMUL, DL, VT, LHS, RHS, Flags);
-    AddToWorklist(Est.getNode());
   }
 
   return Est;
 }
 
 /// Build code to calculate either rsqrt(Op) or sqrt(Op). In the latter case
 /// Op*rsqrt(Op) is actually computed, so additional postprocessing is needed if
 /// Op can be zero.
if (Iterations) {
           // fabs(X) < SmallestNormal ? 0.0 : Est
           const fltSemantics &FltSem = DAG.EVTToAPFloatSemantics(VT);
           APFloat SmallestNorm = APFloat::getSmallestNormalized(FltSem);
           SDValue NormC = DAG.getConstantFP(SmallestNorm, DL, VT);
           SDValue FPZero = DAG.getConstantFP(0.0, DL, VT);
           SDValue Fabs = DAG.getNode(ISD::FABS, DL, VT, Op);
           SDValue IsDenorm = DAG.getSetCC(DL, CCVT, Fabs, NormC, ISD::SETLT);
           Est = DAG.getNode(SelOpcode, DL, VT, IsDenorm, FPZero, Est);
-          AddToWorklist(Fabs.getNode());
-          AddToWorklist(IsDenorm.getNode());
-          AddToWorklist(Est.getNode());
         } else {
           // X == 0.0 ? 0.0 : Est
           SDValue FPZero = DAG.getConstantFP(0.0, DL, VT);
           SDValue IsZero = DAG.getSetCC(DL, CCVT, Op, FPZero, ISD::SETEQ);
           Est = DAG.getNode(SelOpcode, DL, VT, IsZero, FPZero, Est);
-          AddToWorklist(IsZero.getNode());
-          AddToWorklist(Est.getNode());
         }
       }
     }
     return Est;
   }
 
   return SDValue();
 }

llvm/trunk/test/CodeGen/PowerPC/qpx-recipest.ll

 ; CHECK-NEXT:    qvfperm 1, 0, 3, 1
 ; CHECK-NEXT:    blr
 entry:
   %x = call <4 x double> @llvm.sqrt.v4f64(<4 x double> %b)
   %r = fdiv <4 x double> %a, %x
   ret <4 x double> %r
 }
 
-; FIXME: We're currently loading two constants here (1.5 and -1.5), and using
-;        an qvfmadd instead of a qvfnmsubs
 define <4 x double> @foof_fmf(<4 x double> %a, <4 x float> %b) nounwind {
 ; CHECK-LABEL: foof_fmf:
 ; CHECK:       # %bb.0: # %entry
 ; CHECK-NEXT:    addis 3, 2, .LCPI2_0@toc@ha
 ; CHECK-NEXT:    qvfrsqrtes 3, 2
 ; CHECK-NEXT:    addi 3, 3, .LCPI2_0@toc@l
 ; CHECK-NEXT:    qvlfsx 0, 0, 3
-; CHECK-NEXT:    addis 3, 2, .LCPI2_1@toc@ha
-; CHECK-NEXT:    addi 3, 3, .LCPI2_1@toc@l
-; CHECK-NEXT:    qvlfsx 4, 0, 3
-; CHECK-NEXT:    qvfmadds 0, 2, 0, 2
-; CHECK-NEXT:    qvfmuls 2, 3, 3
-; CHECK-NEXT:    qvfmadds 0, 0, 2, 4
+; CHECK-NEXT:    qvfmuls 4, 3, 3
+; CHECK-NEXT:    qvfnmsubs 2, 2, 0, 2
+; CHECK-NEXT:    qvfmadds 0, 2, 4, 0
 ; CHECK-NEXT:    qvfmuls 0, 3, 0
 ; CHECK-NEXT:    qvfmul 1, 1, 0
 ; CHECK-NEXT:    blr
 entry:
   %x = call fast <4 x float> @llvm.sqrt.v4f32(<4 x float> %b)
   %y = fpext <4 x float> %x to <4 x double>
   %r = fdiv fast <4 x double> %a, %y
   ret <4 x double> %r

 ; CHECK-NEXT:    blr
 entry:
   %x = call <4 x double> @llvm.sqrt.v4f64(<4 x double> %b)
   %y = fptrunc <4 x double> %x to <4 x float>
   %r = fdiv <4 x float> %a, %y
   ret <4 x float> %r
 }
 
-; FIXME: We're currently loading two constants here (1.5 and -1.5), and using
-;        an qvfmadd instead of a qvfnmsubs
 define <4 x float> @goo_fmf(<4 x float> %a, <4 x float> %b) nounwind {
 ; CHECK-LABEL: goo_fmf:
 ; CHECK:       # %bb.0: # %entry
 ; CHECK-NEXT:    addis 3, 2, .LCPI6_0@toc@ha
 ; CHECK-NEXT:    qvfrsqrtes 3, 2
 ; CHECK-NEXT:    addi 3, 3, .LCPI6_0@toc@l
 ; CHECK-NEXT:    qvlfsx 0, 0, 3
-; CHECK-NEXT:    addis 3, 2, .LCPI6_1@toc@ha
-; CHECK-NEXT:    addi 3, 3, .LCPI6_1@toc@l
-; CHECK-NEXT:    qvlfsx 4, 0, 3
-; CHECK-NEXT:    qvfmadds 0, 2, 0, 2
-; CHECK-NEXT:    qvfmuls 2, 3, 3
-; CHECK-NEXT:    qvfmadds 0, 0, 2, 4
+; CHECK-NEXT:    qvfmuls 4, 3, 3
+; CHECK-NEXT:    qvfnmsubs 2, 2, 0, 2
+; CHECK-NEXT:    qvfmadds 0, 2, 4, 0
 ; CHECK-NEXT:    qvfmuls 0, 3, 0
 ; CHECK-NEXT:    qvfmuls 1, 1, 0
 ; CHECK-NEXT:    blr
 entry:
   %x = call fast <4 x float> @llvm.sqrt.v4f32(<4 x float> %b)
   %r = fdiv fast <4 x float> %a, %x
   ret <4 x float> %r
 }

 ; CHECK-NEXT:    qvgpci 1, 101
 ; CHECK-NEXT:    qvfperm 1, 0, 2, 1
 ; CHECK-NEXT:    blr
 entry:
   %r = call <4 x double> @llvm.sqrt.v4f64(<4 x double> %a)
   ret <4 x double> %r
 }
 
-; FIXME: We're currently loading two constants here (1.5 and -1.5), and using
-;        an qvfmadds instead of a qvfnmsubs
 define <4 x float> @goo3_fmf(<4 x float> %a) nounwind {
 ; CHECK-LABEL: goo3_fmf:
 ; CHECK:       # %bb.0: # %entry
 ; CHECK-NEXT:    addis 3, 2, .LCPI14_1@toc@ha
 ; CHECK-NEXT:    qvfrsqrtes 2, 1
 ; CHECK-NEXT:    addi 3, 3, .LCPI14_1@toc@l
 ; CHECK-NEXT:    qvlfsx 0, 0, 3
-; CHECK-NEXT:    addis 3, 2, .LCPI14_2@toc@ha
-; CHECK-NEXT:    addi 3, 3, .LCPI14_2@toc@l
-; CHECK-NEXT:    qvlfsx 3, 0, 3
 ; CHECK-NEXT:    addis 3, 2, .LCPI14_0@toc@ha
-; CHECK-NEXT:    qvfmuls 4, 2, 2
 ; CHECK-NEXT:    addi 3, 3, .LCPI14_0@toc@l
-; CHECK-NEXT:    qvfmadds 0, 1, 0, 1
-; CHECK-NEXT:    qvfmadds 0, 0, 4, 3
+; CHECK-NEXT:    qvfmuls 4, 2, 2
+; CHECK-NEXT:    qvfnmsubs 3, 1, 0, 1
+; CHECK-NEXT:    qvfmadds 0, 3, 4, 0
 ; CHECK-NEXT:    qvlfsx 3, 0, 3
 ; CHECK-NEXT:    qvfmuls 0, 2, 0
 ; CHECK-NEXT:    qvfmuls 0, 0, 1
 ; CHECK-NEXT:    qvfcmpeq 1, 1, 3
 ; CHECK-NEXT:    qvfsel 1, 1, 3, 0
 ; CHECK-NEXT:    blr
 entry:
   %r = call fast <4 x float> @llvm.sqrt.v4f32(<4 x float> %a)