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*: "H.J. Lu" <hjl dot tools at gmail dot com>*To*: Roland McGrath <roland at hack dot frob dot com>*Cc*: GNU C Library <libc-alpha at sourceware dot org>*Date*: Wed, 6 Jun 2012 15:56:59 -0700*Subject*: Re: [PATCH] Add x86-64 and SSE math support to i386 bits/mathinline.h*References*: <20120606172605.GA16944@intel.com><20120606215510.83AAA2C09B@topped-with-meat.com>

On Wed, Jun 6, 2012 at 2:55 PM, Roland McGrath <roland@hack.frob.com> wrote: >> @@ -120,26 +119,42 @@ >> >> ?/* The gcc, version 2.7 or below, has problems with all this inlining >> ? ? code. ?So disable it for this version of the compiler. ?*/ >> -# if __GNUC_PREREQ (2, 8) >> +# if __GNUC_PREREQ (2, 8) && defined __USE_ISOC99 >> +__BEGIN_NAMESPACE_C99 > > This is inside: > > ? ? ? ?#if defined __USE_ISOC99 && defined __GNUC__ && __GNUC__ >= 2 > > so the change to the condition is not required. ?If __BEGIN_NAMESPACE_C99 > is necessary that is not strictly part of the x86-{32,64} unification > (though I realize it is a difference between the two existing files) and > so should be in a separate change (and have a bug report). I removed ___USE_ISOC99 and _BEGIN_NAMESPACE_C99. >> ?/* Test for negative number. ?Used in the signbit() macro. ?*/ >> ?__MATH_INLINE int >> ?__NTH (__signbitf (float __x)) >> ?{ >> +# ?ifndef __x86_64__ >> ? ?__extension__ union { float __f; int __i; } __u = { __f: __x }; >> ? ?return __u.__i < 0; >> +# ?else > > It seems more natural to order things as ifdef rather than ifndef, so put > the x86_64 code first. ?But shouldn't this just test __SSE2__ instead? Changed. >> + ?int __m; >> + ?__asm ("pmovmskb %1, %0" : "=r" (__m) : "x" (__x)); >> + ?return __m & 0x8; >> +# ?endif > > The existing i386 function always returns 1 or 0, while this returns 8 or 0. > I realize the existing x86_64 code differs this way too, but that seems > questionable. ?So perhaps this should be "return (__m & 0x8) != 0;"? Changed. >> ?__MATH_INLINE int >> ?__NTH (__signbit (double __x)) >> ?{ >> +# ?ifndef __x86_64__ >> ? ?__extension__ union { double __d; int __i[2]; } __u = { __d: __x }; >> ? ?return __u.__i[1] < 0; >> +# ?else >> + ?int __m; >> + ?__asm ("pmovmskb %1, %0" : "=r" (__m) : "x" (__x)); >> + ?return __m & 0x80; >> +# ?endif > > The same two issues apply here. > Done. >> + ?/* Mark as volatile since the result is dependend on the state of >> + ? ? the SSE control register (the rounding mode). Otherwise GCC might >> + ? ? remove these assembler instructions since it does not know about >> + ? ? the rounding mode change and cannot currently be told. ?*/ > > s/dependend/dependent/. ?There should be two spaces between the sentences. > I realize this is just code you copied from the existing x86_64 file, so > these nits are not your fault per se, but we might as well fix them now. > There are several more instances below which I won't bother to cite > individually. Done. >> +# ? ifdef __SSE2_MATH__ >> +__MATH_INLINE long int >> +__NTH (lrint (double __x)) >> +{ >> + ?long int __res; >> + ?/* Mark as volatile since the result is dependend on the state of >> + ? ? the SSE control register (the rounding mode). Otherwise GCC might >> + ? ? remove these assembler instructions since it does not know about >> + ? ? the rounding mode change and cannot currently be told. ?*/ >> + ?__asm __volatile__ ("cvtsd2si %1, %0" : "=r" (__res) : "xm" (__x)); >> + ?return __res; >> +} >> +# ? endif >> +# ? ifdef __x86_64__ >> +__MATH_INLINE long long int >> +__NTH (llrintf (float __x)) >> +{ >> + ?long long int __res; >> + ?/* Mark as volatile since the result is dependend on the state of >> + ? ? the SSE control register (the rounding mode). Otherwise GCC might >> + ? ? remove these assembler instructions since it does not know about >> + ? ? the rounding mode change and cannot currently be told. ?*/ >> + ?__asm __volatile__ ("cvtss2si %1, %0" : "=r" (__res) : "xm" (__x)); >> + ?return __res; >> +} >> +__MATH_INLINE long long int >> +__NTH (llrint (double __x)) >> +{ >> + ?long long int __res; >> + ?/* Mark as volatile since the result is dependend on the state of >> + ? ? the SSE control register (the rounding mode). Otherwise GCC might >> + ? ? remove these assembler instructions since it does not know about >> + ? ? the rounding mode change and cannot currently be told. ?*/ >> + ?__asm __volatile__ ("cvtsd2si %1, %0" : "=r" (__res) : "xm" (__x)); >> + ?return __res; >> +} >> +# ? endif > > All this duplication can be replaced with an __lrint_code macro like the > existing i386 file uses. > > I don't understand why llrint* are defined this way only for x86_64. > It seems appropriate for __SSE2_MATH__ on i386 as well. 64-bit cvtss2si is only available for x86-64. Is this patch OK? -- H.J.

**Attachment:
0001-Add-x86-64-and-SSE-math-support-to-i386-bits-mathinl.patch**

**References**:**[PATCH] Add x86-64 and SSE math support to i386 bits/mathinline.h***From:*H.J. Lu

**Re: [PATCH] Add x86-64 and SSE math support to i386 bits/mathinline.h***From:*Roland McGrath

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

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