[PATCH] D14327: Add llvm.ldexp.* intrinsic, associated SDNode and library calls

[Hal Finkel via llvm-commits]

hfinkel added inline comments.

================
Comment at: docs/LangRef.rst:9889
@@ +9888,3 @@
+power, thus returning the same values as the libm ``ldexp`` functions
+would.
+
----------------
As I recall, we're very consistent about this, with one exception: @llvm.sqrt. And this causes a lot of confusion. That having been said, there is a precedent, and there are good reasons to do it. However, we do need to say what happens if the result is not representable. You really have two choices:

  1. "and handles error conditions in the same way" (i.e. perhaps sets errno)

   2. Has undefined behavior (it needs to be undefined because it might be implemented using libm, and we can't know whether libm will affect errno)

================
Comment at: lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:3251
@@ +3250,3 @@
+    Tmp1 = DAG.getNode(ISD::SINT_TO_FP, dl, VT, Node->getOperand(1));
+    Tmp2 = DAG.getNode(ISD::FEXP2, dl, VT, Tmp1);
+    Tmp3 = DAG.getNode(ISD::FMUL, dl, VT, Node->getOperand(0), Tmp2);
----------------
This seems like a great idea is FEXP2 is legal, but otherwise, seems likely slower than the original library function call to ldexp. Unless we really know better, we should keep the original call.

================
Comment at: lib/CodeGen/TargetLoweringBase.cpp:873
@@ -860,3 +872,3 @@
   // These library functions default to expand.
   for (MVT VT : {MVT::f32, MVT::f64, MVT::f128}) {
     setOperationAction(ISD::FLOG ,      VT, Expand);
----------------
You should add FLDEXP here too.


http://reviews.llvm.org/D14327