[PATCH] D21127: Remove redundant FMUL in Newton-Raphson SQRT code

[Sanjay Patel via llvm-commits]

spatel added a comment.

Please add a regression test to show the codegen when 2 or more N-R steps are used. PowerPC uses at least 2 refinement steps with 'frsqrte', so it may be easiest to add a sqrt (rather than rsqrt) test to test/CodeGen/PowerPC/recipest.ll instead of adding another RUN to the x86 test.

Please add a regression test to show the x86 codegen with FMA. This can get updated when you do the follow-up patch for isFsqrtCheap() for the faster sqrt/div FPUs in Broadwell, etc.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:355-365
@@ -354,11 +354,13 @@
     SDValue ConstantFoldBITCASTofBUILD_VECTOR(SDNode *, EVT);
     SDValue BuildSDIV(SDNode *N);
     SDValue BuildSDIVPow2(SDNode *N);
     SDValue BuildUDIV(SDNode *N);
     SDValue BuildReciprocalEstimate(SDValue Op, SDNodeFlags *Flags);
     SDValue BuildRsqrtEstimate(SDValue Op, SDNodeFlags *Flags);
-    SDValue BuildRsqrtNROneConst(SDValue Op, SDValue Est, unsigned Iterations,
-                                 SDNodeFlags *Flags);
-    SDValue BuildRsqrtNRTwoConst(SDValue Op, SDValue Est, unsigned Iterations,
-                                 SDNodeFlags *Flags);
+    SDValue BuildSqrtEstimate(SDValue Op, SDNodeFlags *Flags);
+    SDValue BuildSqrtEstimateImpl(SDValue Op, SDNodeFlags *Flags, bool Recip);
+    SDValue BuildSqrtNROneConst(SDValue Op, SDValue Est, unsigned Iterations,
+                                SDNodeFlags *Flags, bool Reciprocal);
+    SDValue BuildSqrtNRTwoConst(SDValue Op, SDValue Est, unsigned Iterations,
+                                SDNodeFlags *Flags, bool Reciprocal);
     SDValue MatchBSwapHWordLow(SDNode *N, SDValue N0, SDValue N1,
----------------
It's a giant mess...but since you're updating these names, I think the recommended solution is to use the current naming convention for these functions - start with a lowercase letter.

================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:14651-14652
@@ +14650,4 @@
+      LHS = DAG.getNode(ISD::FMUL, DL, VT, Est, MinusHalf, Flags);
+    }
+    else {
+      // SQRT: LHS = (A * E) * -0.5
----------------
'else' goes on the same line as '}'. Consider clang-format of the whole patch; there may be other formatting changes needed.

================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:14696-14698
@@ +14695,5 @@
+  SDValue Est = BuildSqrtEstimateImpl(Op, Flags, false);
+  if (!Est) {
+    return SDValue();
+  }
+
----------------
No braces need here; see previous comment about using clang-format.


http://reviews.llvm.org/D21127