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

[Siddhesh Poyarekar <siddhesh at sourceware dot org>]

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