[PATCH] D22975: Add new nodes for computing the Newton series

[Tim Northover via llvm-commits]

t.p.northover added inline comments.

================
Comment at: llvm/include/llvm/CodeGen/ISDOpcodes.h:540-542
@@ -539,1 +539,5 @@
 
+    /// FRECPI - Perform one iteration of the Newton series approximation of
+    /// x^-1.
+    FRECPI,
+    /// FRSQRTI - Perform one iteration of the Newton series approximation of
----------------
You should probably mention the argument order and precision requirements here (specifically that multiple rounding steps may occur).

================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:14522
@@ +14521,3 @@
+
+      if (TLI.hasTargetDAGCombine((ISD::NodeType)ISD::FRECPI))
+        // Newton iterations for reciprocal.
----------------
Isn't this more a question of legality?

================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGDumper.cpp:203-204
@@ -202,2 +202,4 @@
   case ISD::FREM:                       return "frem";
+  case ISD::FRECPI:                     return "FRecpI";
+  case ISD::FRSQRTI:                    return "FRSqrtI";
   case ISD::FCOPYSIGN:                  return "fcopysign";
----------------
Probably should be all lower-case looking at the surrounding entries.

================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:7832-7833
@@ +7831,4 @@
+  // AArch64 reciprocal iteration instruction: (2 - M * N)
+  SDValue NewEst = DAG.getNode(AArch64ISD::FRECPS, DL, VT, Arg, Est, &Flags);
+  NewEst = DAG.getNode(ISD::FMUL, DL, VT, NewEst, Est, &Flags);
+
----------------
Isn't this really just a pattern? It looks like we could mark FRECPI as legal (which is more what the DAGCombiner should be asking anyway) and write:

    def : Pat<(f32 (frecpi f32:$arg, f32:$est),
              (FMULSrr (FRECPS32 $arg, $est) $est>;

Similarly for the sqrt case.

================
Comment at: llvm/test/CodeGen/AArch64/recp-fastmath.ll:16
@@ -15,3 +15,3 @@
 ; CHECK-NEXT: frecpe
-; CHECK-NEXT: fmov
 }
----------------
We should be checking data-flow too in these tests. It's not enough to know that LLVM managed to cobble together an frecps instruction.


Repository:
  rL LLVM

https://reviews.llvm.org/D22975