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] |

*From*: Carlos O'Donell <carlos at redhat dot com>*To*: siddhesh at sourceware dot org, libc-alpha at sourceware dot org*Date*: Thu, 29 Sep 2016 20:37:31 -0400*Subject*: Re: [PATCH 2/4] Check n instead of k1 to decide on sign of sin/cos result*Authentication-results*: sourceware.org; auth=none*References*: <1474998553-2366-1-git-send-email-siddhesh@sourceware.org> <1474998553-2366-3-git-send-email-siddhesh@sourceware.org> <dd631de5-d7b3-41b7-434a-3273bcab1f1e@redhat.com> <6e9f05db-6856-f760-c47c-322e5c141246@sourceware.org>

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.

**References**:**[PATCH 0/4] Use __copysign in sincos wherever possible***From:*Siddhesh Poyarekar

**[PATCH 2/4] Check n instead of k1 to decide on sign of sin/cos result***From:*Siddhesh Poyarekar

**Re: [PATCH 2/4] Check n instead of k1 to decide on sign of sin/cos result***From:*Carlos O'Donell

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

Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|

Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |