This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]