[PATCHv6] New generic sinf

Message ID 1512033760-18792-1-git-send-email-raji@linux.vnet.ibm.com
State Superseded
Headers

Commit Message

Rajalakshmi S Nov. 30, 2017, 9:22 a.m. UTC
  Changes since version 6:

  - Use fabsf instead of fabs.
  - Use unsigned long int instead of unsigned long.
  - Use uint64_t instead of long to handle 32 bit.

---
Changes since version 4:

  - Increase pio2_table size to 5 and add comment.
  - Change exponent calculation to use division.
  - Use isless for comparison.

---
Changes since version 3:

  - Added and corrected comments in reduced.
  - Removed the duplicate theta2 calculation in reduced.

---
Changes since version 2:

  - Added code comments.
  - Added BZ in Changelog.
  - Spacing correction.
  - Added const to theta2 variable.
  - Change in some constant representation.

---
Changes since version 1:

  - Removed sccs id.

---
The same logic used in s_sinf.S version of x86 and powerpc
is moved as generic s_sinf.c, so there is no performance
improvement in x86_64 and powerpc64.
For s390, this is the improvement noted.

With patch:
benchtests/bench-sinf
  "": {
    "duration": 9.91026e+09,
    "iterations": 4.6512e+08,
    "max": 130.26,
    "min": 7.027,
    "mean": 21.3069
   }
Without patch:
 "": {
    "duration": 1.00656e+10,
    "iterations": 1.65699e+08,
    "max": 1740.57,
    "min": 4.729,
    "mean": 60.7461
   }

Also addressed comments from Joseph on generic sincosf version
Ref:https://sourceware.org/ml/libc-alpha/2017-10/msg00367.html

---

This implementation is based on optimized sinf assembly versions
of x86_64 and powerpc.

Tested on s390x, x86_64 and powerpc64le and powerpc32.

2017-11-30  Rajalakshmi Srinivasaraghavan  <raji@linux.vnet.ibm.com>

 	[BZ #5997]
	* sysdeps/ieee754/flt-32/s_sinf.c: New implementation.
---
 sysdeps/ieee754/flt-32/s_sinf.c | 267 ++++++++++++++++++++++++++++++++++------
 1 file changed, 226 insertions(+), 41 deletions(-)
  

Comments

Joseph Myers Nov. 30, 2017, 5:14 p.m. UTC | #1
On Thu, 30 Nov 2017, Rajalakshmi Srinivasaraghavan wrote:

> +	      exponent =
> +	        (exponent >> FLOAT_EXPONENT_SHIFT) - FLOAT_EXPONENT_BIAS;

Break lines before operators, not after.

OK with that fix (remember of course to resolve bug 5997 as FIXED with 
target milestone set to 2.27 when checking in the patch).
  
Paul A. Clarke Dec. 1, 2017, 1:17 a.m. UTC | #2
On 11/30/2017 03:22 AM, Rajalakshmi Srinivasaraghavan wrote:
...snip...

> +float
> +SINF_FUNC (float x)
> +{
> +  double cx;
> +  double theta = x;
> +  double abstheta = fabs (theta);
> +  /* If |x|< Pi/4.  */
> +  if (abstheta < M_PI_4)
> +    {
> +      if (abstheta >= 0x1p-5) /* |x| >= 2^-5.  */
> +	{
> +	  const double theta2 = theta * theta;
> +	  /* Chebyshev polynomial of the form for sin
> +	     x+x^3*(S0+x^2*(S1+x^2*(S2+x^2*(S3+x^2*S4)))).  */
> +	  cx = S3 + theta2 * S4;
> +	  cx = S2 + theta2 * cx;
> +	  cx = S1 + theta2 * cx;
> +	  cx = S0 + theta2 * cx;
> +	  cx = theta + theta * theta2 * cx;
> +	  return cx;
> +	}
> +      else if (abstheta >= 0x1p-27)     /* |x| >= 2^-27.  */
> +	{
> +	  /* A simpler Chebyshev approximation is close enough for this range:
> +	     for sin: x+x^3*(SS0+x^2*SS1).  */
> +	  const double theta2 = theta * theta;
> +	  cx = SS0 + theta2 * SS1;
> +	  cx = theta + theta * theta2 * cx;
> +	  return cx;
> +	}
> +      else
> +	{
> +	  /* Handle some special cases.  */
> +	  if (theta)
> +	    return theta - (theta * SMALL);
> +	  else
> +	    return theta;
> +	}
> +    }
> +  else                          /* |x| >= Pi/4.  */
> +    {
> +      unsigned long int signbit = (x < 0);
> +      if (abstheta < 9 * M_PI_4)        /* |x| < 9*Pi/4.  */
> +	{
> +	  /* There are cases where FE_UPWARD rounding mode can
> +	     produce a result of abstheta * inv_PI_4 == 9,
> +	     where abstheta < 9pi/4, so the domain for
> +	     pio2_table must go to 5 (9 / 2 + 1).  */
> +	  unsigned long int n = (abstheta * inv_PI_4) + 1;
> +	  theta = abstheta - pio2_table[n / 2];
> +	  return reduced (theta, n, signbit);
> +	}
> +      else if (isless (abstheta, INFINITY))
> +	{
> +	  if (abstheta < 0x1p+23)     /* |x| < 2^23.  */
> +	    {
> +	      unsigned long int n = floor (abstheta * inv_PI_4) + 1.0;
> +	      double x = floor (n / 2.0);
> +	      theta = x * PI_2_lo + (x * PI_2_hi + abstheta);
> +	      /* Argument reduction needed.  */
> +	      return reduced (theta, n, signbit);
> +	    }
> +	  else                  /* |x| >= 2^23.  */
> +	    {
> +	      x = fabsf (x);
> +	      int exponent;
> +	      GET_FLOAT_WORD (exponent, x);
> +	      exponent =
> +	        (exponent >> FLOAT_EXPONENT_SHIFT) - FLOAT_EXPONENT_BIAS;
> +	      exponent += 3;
> +	      exponent /= 28;
> +	      double a = invpio4_table[exponent] * x;
> +	      double b = invpio4_table[exponent + 1] * x;
> +	      double c = invpio4_table[exponent + 2] * x;
> +	      double d = invpio4_table[exponent + 3] * x;
> +	      uint64_t l = a;
> +	      l &= ~0x7;
> +	      a -= l;
> +	      double e = a + b;
> +	      l = e;
> +	      e = a - l;
> +	      if (l & 1)

The next 20+ lines have 8 spaces (after an initial tab) where a tab should go (if I understand whitespace conventions correctly).  From here...

> +	        {
> +	          e -= 1.0;
> +	          e += b;
> +	          e += c;
> +	          e += d;
> +	          e *= M_PI_4;
> +	          return reduced (e, l + 1, signbit);
> +	        }
> +	      else
> +	        {
> +	          e += b;
> +	          e += c;
> +	          e += d;
> +	          if (e <= 1.0)
> +	            {
> +	              e *= M_PI_4;
> +	              return reduced (e, l + 1, signbit);
> +	            }
> +	          else
> +	            {
> +	              l++;
> +	              e -= 2.0;
> +	              e *= M_PI_4;
> +	              return reduced (e, l + 1, signbit);
> +	            }
> +	        }

...to here.

>  	    }
>  	}
> +      else
> +	{
> +	  int32_t ix;
> +	  /* High word of x.  */
> +	  GET_FLOAT_WORD (ix, abstheta);
> +	  /* Sin(Inf or NaN) is NaN.  */
> +	  if (ix == 0x7f800000)
> +	    __set_errno (EDOM);
> +	  return x - x;
> +	}
> +    }
>  }

PC
  
Rajalakshmi S Dec. 4, 2017, 5:38 a.m. UTC | #3
On 11/30/2017 10:44 PM, Joseph Myers wrote:
> On Thu, 30 Nov 2017, Rajalakshmi Srinivasaraghavan wrote:
> 
>> +	      exponent =
>> +	        (exponent >> FLOAT_EXPONENT_SHIFT) - FLOAT_EXPONENT_BIAS;
> 
> Break lines before operators, not after.

Pushed as 7863a7118112fe502e8020a0db0fa74fef281f29 addressing this 
comment and Paul Clarke's comments on v6 patch.

> 
> OK with that fix (remember of course to resolve bug 5997 as FIXED with
> target milestone set to 2.27 when checking in the patch).

Thanks for the review.

>
  
Szabolcs Nagy Dec. 5, 2017, 5:23 p.m. UTC | #4
On 30/11/17 09:22, Rajalakshmi Srinivasaraghavan wrote:
> Changes since version 4:
> 
>   - Increase pio2_table size to 5 and add comment.
>   - Change exponent calculation to use division.
>   - Use isless for comparison.

isless should be used for all the compares that may compare nan.

i get

testing float (without inline functions)
Failure: sin (qNaN): Exception "Invalid operation" set
Failure: sin (-qNaN): Exception "Invalid operation" set
Failure: sin_downward (qNaN): Exception "Invalid operation" set
Failure: sin_downward (-qNaN): Exception "Invalid operation" set
Failure: sin_towardzero (qNaN): Exception "Invalid operation" set
Failure: sin_towardzero (-qNaN): Exception "Invalid operation" set
Failure: sin_upward (qNaN): Exception "Invalid operation" set
Failure: sin_upward (-qNaN): Exception "Invalid operation" set

> +float
> +SINF_FUNC (float x)
> +{
> +  double cx;
> +  double theta = x;
> +  double abstheta = fabs (theta);
> +  /* If |x|< Pi/4.  */
> +  if (abstheta < M_PI_4)
> +    {
...
> +    }
> +  else                          /* |x| >= Pi/4.  */
> +    {
> +      unsigned long int signbit = (x < 0);
> +      if (abstheta < 9 * M_PI_4)        /* |x| < 9*Pi/4.  */
> +	{
...
> +	}
> +      else if (isless (abstheta, INFINITY))
> +	{
...
>  	}
> +      else
> +	{
> +	  int32_t ix;
> +	  /* High word of x.  */
> +	  GET_FLOAT_WORD (ix, abstheta);
> +	  /* Sin(Inf or NaN) is NaN.  */
> +	  if (ix == 0x7f800000)
> +	    __set_errno (EDOM);
> +	  return x - x;
> +	}
> +    }
>  }
  
Joseph Myers Dec. 5, 2017, 7:03 p.m. UTC | #5
You should also add a NEWS entry for this optimized function (or add it to 
the existing NEWS entry for various optimized float functions).
  
Rajalakshmi S Dec. 6, 2017, 6:43 a.m. UTC | #6
On 12/06/2017 12:33 AM, Joseph Myers wrote:
> You should also add a NEWS entry for this optimized function (or add it to
> the existing NEWS entry for various optimized float functions).
> 

Added it.
  

Patch

diff --git a/sysdeps/ieee754/flt-32/s_sinf.c b/sysdeps/ieee754/flt-32/s_sinf.c
index 3ec98f811d..6f347d1571 100644
--- a/sysdeps/ieee754/flt-32/s_sinf.c
+++ b/sysdeps/ieee754/flt-32/s_sinf.c
@@ -1,21 +1,20 @@ 
-/* s_sinf.c -- float version of s_sin.c.
- * Conversion to float by Ian Lance Taylor, Cygnus Support, ian@cygnus.com.
- */
-
-/*
- * ====================================================
- * Copyright (C) 1993 by Sun Microsystems, Inc. All rights reserved.
- *
- * Developed at SunPro, a Sun Microsystems, Inc. business.
- * Permission to use, copy, modify, and distribute this
- * software is freely granted, provided that this notice
- * is preserved.
- * ====================================================
- */
-
-#if defined(LIBM_SCCS) && !defined(lint)
-static char rcsid[] = "$NetBSD: s_sinf.c,v 1.4 1995/05/10 20:48:16 jtc Exp $";
-#endif
+/* Compute sine of argument.
+   Copyright (C) 2017 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
 
 #include <errno.h>
 #include <math.h>
@@ -28,35 +27,221 @@  static char rcsid[] = "$NetBSD: s_sinf.c,v 1.4 1995/05/10 20:48:16 jtc Exp $";
 # define SINF_FUNC SINF
 #endif
 
-float SINF_FUNC(float x)
-{
-	float y[2],z=0.0;
-	int32_t n, ix;
+/* Chebyshev constants for cos, range -PI/4 - PI/4.  */
+static const double C0 = -0x1.ffffffffe98aep-2;
+static const double C1 =  0x1.55555545c50c7p-5;
+static const double C2 = -0x1.6c16b348b6874p-10;
+static const double C3 =  0x1.a00eb9ac43ccp-16;
+static const double C4 = -0x1.23c97dd8844d7p-22;
 
-	GET_FLOAT_WORD(ix,x);
+/* Chebyshev constants for sin, range -PI/4 - PI/4.  */
+static const double S0 = -0x1.5555555551cd9p-3;
+static const double S1 =  0x1.1111110c2688bp-7;
+static const double S2 = -0x1.a019f8b4bd1f9p-13;
+static const double S3 =  0x1.71d7264e6b5b4p-19;
+static const double S4 = -0x1.a947e1674b58ap-26;
 
-    /* |x| ~< pi/4 */
-	ix &= 0x7fffffff;
-	if(ix <= 0x3f490fd8) return __kernel_sinf(x,z,0);
+/* Chebyshev constants for sin, range 2^-27 - 2^-5.  */
+static const double SS0 = -0x1.555555543d49dp-3;
+static const double SS1 =  0x1.110f475cec8c5p-7;
 
-    /* sin(Inf or NaN) is NaN */
-	else if (ix>=0x7f800000) {
-	  if (ix == 0x7f800000)
-	    __set_errno (EDOM);
-	  return x-x;
-	}
+/* PI/2 with 98 bits of accuracy.  */
+static const double PI_2_hi = -0x1.921fb544p+0;
+static const double PI_2_lo = -0x1.0b4611a626332p-34;
+
+static const double SMALL = 0x1p-50; /* 2^-50.  */
+static const double inv_PI_4 = 0x1.45f306dc9c883p+0; /* 4/PI.  */
+
+#define FLOAT_EXPONENT_SHIFT 23
+#define FLOAT_EXPONENT_BIAS 127
+
+static const double pio2_table[] = {
+  0 * M_PI_2,
+  1 * M_PI_2,
+  2 * M_PI_2,
+  3 * M_PI_2,
+  4 * M_PI_2,
+  5 * M_PI_2
+};
+
+static const double invpio4_table[] = {
+  0x0p+0,
+  0x1.45f306cp+0,
+  0x1.c9c882ap-28,
+  0x1.4fe13a8p-58,
+  0x1.f47d4dp-85,
+  0x1.bb81b6cp-112,
+  0x1.4acc9ep-142,
+  0x1.0e4107cp-169
+};
+
+static const int ones[] = { +1, -1 };
 
-    /* argument reduction needed */
-	else {
-	    n = __ieee754_rem_pio2f(x,y);
-	    switch(n&3) {
-		case 0: return  __kernel_sinf(y[0],y[1],1);
-		case 1: return  __kernel_cosf(y[0],y[1]);
-		case 2: return -__kernel_sinf(y[0],y[1],1);
-		default:
-			return -__kernel_cosf(y[0],y[1]);
+/* Compute the sine value using Chebyshev polynomials where
+   THETA is the range reduced absolute value of the input
+   and it is less than Pi/4,
+   N is calculated as trunc(|x|/(Pi/4)) + 1 and it is used to decide
+   whether a sine or cosine approximation is more accurate and
+   SIGNBIT is used to add the correct sign after the Chebyshev
+   polynomial is computed.  */
+static inline float
+reduced (const double theta, const unsigned long int n,
+	 const unsigned long int signbit)
+{
+  double sx;
+  const double theta2 = theta * theta;
+  /* We are operating on |x|, so we need to add back the original
+     signbit for sinf.  */
+  int sign;
+  /* Determine positive or negative primary interval.  */
+  sign = ones[((n >> 2) & 1) ^ signbit];
+  /* Are we in the primary interval of sin or cos?  */
+  if ((n & 2) == 0)
+    {
+      /* Here sinf() is calculated using sin Chebyshev polynomial:
+	x+x^3*(S0+x^2*(S1+x^2*(S2+x^2*(S3+x^2*S4)))).  */
+      sx = S3 + theta2 * S4;     /* S3+x^2*S4.  */
+      sx = S2 + theta2 * sx;     /* S2+x^2*(S3+x^2*S4).  */
+      sx = S1 + theta2 * sx;     /* S1+x^2*(S2+x^2*(S3+x^2*S4)).  */
+      sx = S0 + theta2 * sx;     /* S0+x^2*(S1+x^2*(S2+x^2*(S3+x^2*S4))).  */
+      sx = theta + theta * theta2 * sx;
+    }
+  else
+    {
+     /* Here sinf() is calculated using cos Chebyshev polynomial:
+	1.0+x^2*(C0+x^2*(C1+x^2*(C2+x^2*(C3+x^2*C4)))).  */
+      sx = C3 + theta2 * C4;     /* C3+x^2*C4.  */
+      sx = C2 + theta2 * sx;     /* C2+x^2*(C3+x^2*C4).  */
+      sx = C1 + theta2 * sx;     /* C1+x^2*(C2+x^2*(C3+x^2*C4)).  */
+      sx = C0 + theta2 * sx;     /* C0+x^2*(C1+x^2*(C2+x^2*(C3+x^2*C4))).  */
+      sx = 1.0 + theta2 * sx;
+    }
+
+  /* Add in the signbit and assign the result.  */
+  return sign * sx;
+}
+
+float
+SINF_FUNC (float x)
+{
+  double cx;
+  double theta = x;
+  double abstheta = fabs (theta);
+  /* If |x|< Pi/4.  */
+  if (abstheta < M_PI_4)
+    {
+      if (abstheta >= 0x1p-5) /* |x| >= 2^-5.  */
+	{
+	  const double theta2 = theta * theta;
+	  /* Chebyshev polynomial of the form for sin
+	     x+x^3*(S0+x^2*(S1+x^2*(S2+x^2*(S3+x^2*S4)))).  */
+	  cx = S3 + theta2 * S4;
+	  cx = S2 + theta2 * cx;
+	  cx = S1 + theta2 * cx;
+	  cx = S0 + theta2 * cx;
+	  cx = theta + theta * theta2 * cx;
+	  return cx;
+	}
+      else if (abstheta >= 0x1p-27)     /* |x| >= 2^-27.  */
+	{
+	  /* A simpler Chebyshev approximation is close enough for this range:
+	     for sin: x+x^3*(SS0+x^2*SS1).  */
+	  const double theta2 = theta * theta;
+	  cx = SS0 + theta2 * SS1;
+	  cx = theta + theta * theta2 * cx;
+	  return cx;
+	}
+      else
+	{
+	  /* Handle some special cases.  */
+	  if (theta)
+	    return theta - (theta * SMALL);
+	  else
+	    return theta;
+	}
+    }
+  else                          /* |x| >= Pi/4.  */
+    {
+      unsigned long int signbit = (x < 0);
+      if (abstheta < 9 * M_PI_4)        /* |x| < 9*Pi/4.  */
+	{
+	  /* There are cases where FE_UPWARD rounding mode can
+	     produce a result of abstheta * inv_PI_4 == 9,
+	     where abstheta < 9pi/4, so the domain for
+	     pio2_table must go to 5 (9 / 2 + 1).  */
+	  unsigned long int n = (abstheta * inv_PI_4) + 1;
+	  theta = abstheta - pio2_table[n / 2];
+	  return reduced (theta, n, signbit);
+	}
+      else if (isless (abstheta, INFINITY))
+	{
+	  if (abstheta < 0x1p+23)     /* |x| < 2^23.  */
+	    {
+	      unsigned long int n = floor (abstheta * inv_PI_4) + 1.0;
+	      double x = floor (n / 2.0);
+	      theta = x * PI_2_lo + (x * PI_2_hi + abstheta);
+	      /* Argument reduction needed.  */
+	      return reduced (theta, n, signbit);
+	    }
+	  else                  /* |x| >= 2^23.  */
+	    {
+	      x = fabsf (x);
+	      int exponent;
+	      GET_FLOAT_WORD (exponent, x);
+	      exponent =
+	        (exponent >> FLOAT_EXPONENT_SHIFT) - FLOAT_EXPONENT_BIAS;
+	      exponent += 3;
+	      exponent /= 28;
+	      double a = invpio4_table[exponent] * x;
+	      double b = invpio4_table[exponent + 1] * x;
+	      double c = invpio4_table[exponent + 2] * x;
+	      double d = invpio4_table[exponent + 3] * x;
+	      uint64_t l = a;
+	      l &= ~0x7;
+	      a -= l;
+	      double e = a + b;
+	      l = e;
+	      e = a - l;
+	      if (l & 1)
+	        {
+	          e -= 1.0;
+	          e += b;
+	          e += c;
+	          e += d;
+	          e *= M_PI_4;
+	          return reduced (e, l + 1, signbit);
+	        }
+	      else
+	        {
+	          e += b;
+	          e += c;
+	          e += d;
+	          if (e <= 1.0)
+	            {
+	              e *= M_PI_4;
+	              return reduced (e, l + 1, signbit);
+	            }
+	          else
+	            {
+	              l++;
+	              e -= 2.0;
+	              e *= M_PI_4;
+	              return reduced (e, l + 1, signbit);
+	            }
+	        }
 	    }
 	}
+      else
+	{
+	  int32_t ix;
+	  /* High word of x.  */
+	  GET_FLOAT_WORD (ix, abstheta);
+	  /* Sin(Inf or NaN) is NaN.  */
+	  if (ix == 0x7f800000)
+	    __set_errno (EDOM);
+	  return x - x;
+	}
+    }
 }
 
 #ifndef SINF