Re: [PATCHv4] New generic sinf

[Joseph Myers <joseph at codesourcery dot com>]

On Fri, 3 Nov 2017, Rajalakshmi Srinivasaraghavan wrote:

> +  /*  if |x|< Pi/4.  */

There should only be one space at the start of a comment, and it should 
start with a capital letter.

> +      if (abstheta < 9 * M_PI_4)        /* |x| < 9*Pi/4.  */
> +	{
> +	  unsigned long n = (abstheta * inv_PI_4) + 1;
> +	  theta = abstheta - pio2_table[n / 2];

Remark: this is only safe because abstheta is a value representable as 
float - specifically, because no double values that are below the 
FE_UPWARD value of 9 * M_PI_4, but still have the FE_UPWARD value of 
abstheta * inv_PI_4 being >= 9, are representable as float.  (There is 
exactly one such double value, 0x1.c463abeccb2bap+2.)  I think a comment 
to this effect, about why the conditional is sufficient to ensure that 
pio2_table[5] does not get accessed, is appropriate.

> +      else if (abstheta < INFINITY)

This would create spurious exceptions for quiet NaN arguments.  You need 
an unordered comparison using isless.  (You tested on exactly the 
architectures where GCC bugs mean unordered comparisons are wrongly used 
for ordered comparison operators and so this issue was hidden - bug 58684 
for powerpc, bug 52451 for x86_64/x86 before GCC 8, bug 77918 for s390.)

> +	      exponent = (exponent * (0x100000000 / 28 + 1)) >> 32;

It's the compiler's job to convert division into a multiplication if 
appropriate, depending on what's optimal for a given architecture.  You 
should write whatever division you mean.  If the compiler could reasonably 
simplify things (including using whatever range information it has for 
exponent) but does not, that should be filed as a compiler bug.  Only if 
the compiler can't reasonably determine how to express a given division as 
a multiplication should the C code in glibc maybe express it as a 
multiplication like this (and in that case a detailed comment explaining 
the issues should be present).

-- 
Joseph S. Myers
joseph@codesourcery.com