[libc-commits] [PATCH] D123154: [libc] Implement sinf function that is correctly rounded to all rounding modes.

[Tue Ly via Phabricator via libc-commits]

lntue added inline comments.


================
Comment at: libc/src/math/generic/range_reduction.h:33-40
+static constexpr double ONE_OVER_PI_28[N_ENTRIES] = {
+    0x1.45f306ep-2,   -0x1.b1bbeaep-33,  0x1.3f84ebp-62,    -0x1.7056592p-92,
+    0x1.c0db62ap-121, -0x1.4cd8778p-150, -0x1.bef806cp-179, 0x1.63abdecp-209};
+
+// Exponents of the least significant bits of the corresponding entries in
+// ONE_OVER_PI_28.
+static constexpr int ONE_OVER_PI_28_LSB_EXP[N_ENTRIES] = {
----------------
santoshn wrote:
> santoshn wrote:
> > lntue wrote:
> > > santoshn wrote:
> > > > I was reviewing this code. I am not able to understand how ONE_OVER_PI_28_LSB_EXP[2] == -86.  The third entry in ONE_OVER_PI_28 (i.e., 0x1.3f84ebp-62)  has an exponent of -62 and there are 27 fraction bits. So the exponent of the LSB should be -89.
> > > > 
> > > >  Similarly  ONE_OVER_PI_28_LSB_EXP[5] is -167?
> > > > 
> > > > Similarly ONE_OVER_PI_28_LSB_EXP[6] is -206?
> > > > 
> > > > Similarly ONE_OVER_PI_28_LSB_EXP[7] is -236?
> > > I think for `ONE_OVER_PI_28_LSB_EXP[2]` I over-counted a bit.  The exact value `0x1.3f84eb p-62` actually has the last 1 bit at 24th bit after the decimal point (bits 25-27 are all 0's), so the correct value of `ONE_VER_PI_28_LSB_EXP[2]` should be `-62 - 24 = -66`.  Similarly for other values.
> > Tue,
> > 
> > Thanks for the clarification. 
> > 
> > The last hex digit in ONE_OVER_PI_28_LSP[2]  is "b", which is 1011. We are using 28-bits of precision for each entry of OVER_OVER_PI_28
> > I get -62-27 = -89. 
> The ONE_OVER_PI_28_LSB_EXP array according to me for the double values with a precision of 28 for ONE_OVER_PI_28 should be.
> 
> static const double one_over_pi_28_exp[8] = {
>    -29, //  -2 -27 = -29
>    -60,  // -33 -27 = -60
>    -89,  // -62 -27 = -89, last hex digit is b which is  1011
>    -118, // -92 - 26 = -118, last hex digit is 2, which is  0010
>    -147, // -121 -26  = -147, last hex digit is a, which is 1010
>    -174, // -150-24 = -174, last hex digit is 8, which is 1000
>    -204, // -179-25 = -204, last hex digit is c, which is 1100
>    -234 // -209 -25 = -234, last hex digit is c, which is 1100
>   };
> 
> In the RLIBM project, we have  a similar version range reduction for sinf. We preform two levels of range reduction.  The first level of range reduction is exactly similar to the approach here. Second level approximates sinpi(x), with two polynomials sinpi and cospi, similar to the description in Section 2 of our  PLDI 2021 paper: https://people.cs.rutgers.edu/~sn349/papers/rlibm32-pldi-2021-preprint.pdf 
> 
> Hence, we think the performance of this sinf function can be significantly improved (close to 2X improvement).
> 
> We are doing final stages of testing for our sinf polynomial, which consist of two polynomials: a 3 term, degree-5 polynomial (sinpi) and a 3-term, degree-4 polynomial (cospi) and a table of 512 double values.
> 
> We will share the link to the implementation here once we are done with our testing. Hope it can be incorporated. 
> 
> 
Yes, the last hex digit is `b`, but it has only 6 hex digits, and hence `-24` instead of `-27`;  a full 7 hex digits will make it `-28` instead.

It would be great to be able to reduce the polynomial's degree + exceptional cases.  Thanks for letting me know the progress!  I'm looking forward to it!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123154/new/

https://reviews.llvm.org/D123154