[2/4] Check n instead of k1 to decide on sign of sin/cos result

  For k1 in 1 and 3, n can only have values of 0 and 2, so checking k1 &
2 is equivalent to checking n & 2.  We prefer the latter so that we
don't use k1 for anything other than selecting the quadrant in
do_sincos_1, thus dropping it completely.

	* sysdeps/ieee754/dbl-64/s_sin.c (do_sincos_1): Check N
	instead of K1.
---
 sysdeps/ieee754/dbl-64/s_sin.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
  

On 09/27/2016 01:49 PM, Siddhesh Poyarekar wrote:
> For k1 in 1 and 3, n can only have values of 0 and 2, so checking k1 &
> 2 is equivalent to checking n & 2.  We prefer the latter so that we
> don't use k1 for anything other than selecting the quadrant in
> do_sincos_1, thus dropping it completely.

This is only true of the quadrant shift K is restricted to 0 or 1, which
is true of the code today, but may not be true tomorrow. 

In which case you need `assert (k == 0 || k == 1);` and a comment
about this function only working for a shift of 0 or 1.

OK to checkin if you add the assert and comment.

> 	* sysdeps/ieee754/dbl-64/s_sin.c (do_sincos_1): Check N
> 	instead of K1.
> ---
>  sysdeps/ieee754/dbl-64/s_sin.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sysdeps/ieee754/dbl-64/s_sin.c b/sysdeps/ieee754/dbl-64/s_sin.c
> index d60feb4..ea41a7c 100644
> --- a/sysdeps/ieee754/dbl-64/s_sin.c
> +++ b/sysdeps/ieee754/dbl-64/s_sin.c
> @@ -353,7 +353,7 @@ do_sincos_1 (double a, double da, double x, int4 n, int4 k)
>      case 3:
>        res = do_cos (a, da, &cor);
>        cor = (cor > 0) ? 1.025 * cor + eps : 1.025 * cor - eps;
> -      retval = ((res == res + cor) ? ((k1 & 2) ? -res : res)
> +      retval = ((res == res + cor) ? ((n & 2) ? -res : res)
>  		: sloww2 (a, da, x, n));
>        break;
>      }
> 

Start with:
(k1 & 2)

Expand in:
k1 = (n + k) & 3
(((n + k) & 3) & 2)

Simplify:
(n + k) & 2

Assume k1 is 1 or 3:
(n + k) & 3 = 1
(n + k) & 3 = 3

Requires the following ranges:
n = 0, k = 1
n = 1, k = 0

n = 0, k = 3
n = 1, k = 2
n = 2, k = 1
n = 3, k = 0


Produces the following result:
n = 0, k = 1, (n + k) & 2 = 0, n & 2 = 0; PASS
n = 1, k = 0, (n + k) & 2 = 0, n & 2 = 0; PASS

n = 0, k = 3, (n + k) & 2 = 2, n & 2 = 0; FAIL
n = 1, k = 2, (n + k) & 2 = 2, n & 2 = 0; FAIL
n = 2, k = 1, (n + k) & 2 = 2, n & 2 = 2; PASS
n = 3, k = 0, (n + k) & 2 = 2, n & 2 = 2; PASS

Thus the substitution you make is invalid for case where K is 2 or 3,
but that's never used in the code.
  

On Wednesday 28 September 2016 12:54 PM, Carlos O'Donell wrote:
> On 09/27/2016 01:49 PM, Siddhesh Poyarekar wrote:
>> For k1 in 1 and 3, n can only have values of 0 and 2, so checking k1 &
>> 2 is equivalent to checking n & 2.  We prefer the latter so that we
>> don't use k1 for anything other than selecting the quadrant in
>> do_sincos_1, thus dropping it completely.
> 
> This is only true of the quadrant shift K is restricted to 0 or 1, which
> is true of the code today, but may not be true tomorrow. 
> 
> In which case you need `assert (k == 0 || k == 1);` and a comment
> about this function only working for a shift of 0 or 1.
> 
> OK to checkin if you add the assert and comment.

do_sincos_1 with K values other than 0 or 1 does not make sense in the
context of sin/cos because they're only 1 quadrant apart.  To be clear,
the previous logic was:

    "Compute sine for the value and based on the new rotated quadrant
     (k1) negate the value if we're in the fourth quadrant."

With the change, the logic is:

    "Compute sine for the value and negate it if we were either (1) in
     the fourth quadrant or (2) we actually wanted the cosine and were
     in the third quadrant."

which should be more intuitive when you visualize the quadrants. I can
update the patch description with the above.  I can add a comment to
do_sincos_* functions explicitly stating that K can only be either 0 and
1.  I can even make K an enum {CURRENT_QUADRANT = 0, NEXT_QUADRANT = 1}
to make it explicit in code that we don't want any other values there.
The assert is an added instruction that I want to avoid, but maybe it'll
get optimized away given that the functions are inlined anyway.

What do you prefer?

Siddhesh
  

On 09/28/2016 05:57 PM, Siddhesh Poyarekar wrote:
> On Wednesday 28 September 2016 12:54 PM, Carlos O'Donell wrote:
>> On 09/27/2016 01:49 PM, Siddhesh Poyarekar wrote:
>>> For k1 in 1 and 3, n can only have values of 0 and 2, so checking k1 &
>>> 2 is equivalent to checking n & 2.  We prefer the latter so that we
>>> don't use k1 for anything other than selecting the quadrant in
>>> do_sincos_1, thus dropping it completely.
>>
>> This is only true of the quadrant shift K is restricted to 0 or 1, which
>> is true of the code today, but may not be true tomorrow. 
>>
>> In which case you need `assert (k == 0 || k == 1);` and a comment
>> about this function only working for a shift of 0 or 1.
>>
>> OK to checkin if you add the assert and comment.
> 
> do_sincos_1 with K values other than 0 or 1 does not make sense in the
> context of sin/cos because they're only 1 quadrant apart.  To be clear,
> the previous logic was:
> 
>     "Compute sine for the value and based on the new rotated quadrant
>      (k1) negate the value if we're in the fourth quadrant."
> 
> With the change, the logic is:
> 
>     "Compute sine for the value and negate it if we were either (1) in
>      the fourth quadrant or (2) we actually wanted the cosine and were
>      in the third quadrant."
> 
> which should be more intuitive when you visualize the quadrants. I can
> update the patch description with the above.  I can add a comment to
> do_sincos_* functions explicitly stating that K can only be either 0 and
> 1.  I can even make K an enum {CURRENT_QUADRANT = 0, NEXT_QUADRANT = 1}
> to make it explicit in code that we don't want any other values there.
> The assert is an added instruction that I want to avoid, but maybe it'll
> get optimized away given that the functions are inlined anyway.
> 
> What do you prefer?

I like where you are going with this.

The simplest solution I think is:

* Make K a bool (C99 bool type), which should resolve the issue.
  The compiler will always only use 0 or 1 and the result works.
* Update with the description above.

OK with that.
  

On Friday 30 September 2016 06:07 AM, Carlos O'Donell wrote:
> I like where you are going with this.
> 
> The simplest solution I think is:
> 
> * Make K a bool (C99 bool type), which should resolve the issue.
>   The compiler will always only use 0 or 1 and the result works.

Ack, I made that a separate patch and pushed it (see other [committed]
email)

> * Update with the description above.

Done and pushed.

Thanks,
Siddhesh