This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH 2/4] Check n instead of k1 to decide on sign of sin/cos result
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.
--
Cheers,
Carlos.