Re: PATCH: SSE sin and cos for x86 and x86_64

[Andreas Jaeger <aj at suse dot com>]

On Thursday, August 23, 2012 17:28:26 Dmitrieva Liubov wrote:
> Ping.
> 
> SSE2 sin and cos patches (up to 10 times faster) are waiting for
> review and my patch with new test cases as well.
> I've attached all 3 again (the same as last time)

The patches are all fine with one minor nit: Please reformat them so that 
we do not have overlong lines (> 78 chars). A couple of comments are 
going over.

Note: do not wrap libm-test.inc, those long lines are fine.
> #1
> 2012-08-16  Liubov Dmitrieva  <liubov.dmitrieva@gmail.com>
> 
>           * math/libm-test.inc: Update
>           Add new test cases in large arguments path.

This should name the functions you change:
           * math/libm-test.inc (cos_test): Add more test cases.
		(sin_test): Likewise.
		(sincos_test): Likewise.


 
> #2
> 2012-08-13  Liubov Dmitrieva  <liubov.dmitrieva@gmail.com>
> 
>          * sysdeps/i386/i686/fpu/multiarch/Makefile: Update
>          (sysdep_routines): Add s_sinf-sse2, s_conf-sse2


Just name the variable you update, no need to say "Update" for the file:

          * sysdeps/i386/i686/fpu/multiarch/Makefile (sysdep_routines): 	
         Add s_sinf-sse2, s_conf-sse2

> 
>          * sysdeps/i386/i686/fpu/multiarch/s_sinf-sse2.S: New file
>          * sysdeps/i386/i686/fpu/multiarch/s_cosf-sse2.S: New file
>          * sysdeps/i386/i686/fpu/multiarch/s_sinf.c: New file
>          * sysdeps/i386/i686/fpu/multiarch/s_cosf.c: New file
>          * sysdeps/ieee754/flt-32/s_sinf.c: Update
>          (SINF): Add macro for using routine as __sinf_ia32

see below
>          * sysdeps/ieee754/flt-32/s_cosf.c: Update
>          (COSF): Add macro for using routine as __cosf_ia32

Don't update the Sun copyrights.
And write the changelog as:
          * sysdeps/ieee754/flt-32/s_cosf.c (COSF, COFS_FUNC): Add macros 
for using routine as __cosf_ia32.
	    Use macro for function declaration and weak_alias.



 
>          * sysdeps/i386/i686/fpu/multiarch/e_expf-sse2.S: Fix
> Copyright * sysdeps/i386/i686/fpu/multiarch/e_expf.c: Fix Copyright
> 
> #3
> 2012-08-15  Liubov Dmitrieva  <liubov.dmitrieva@gmail.com>
> 
>        * sysdeps/x86_64/fpu/s_sinf.S: New file.
>        * sysdeps/x86_64/fpu/s_cosf.S: New file.
>        * sysdeps/x86_64/fpu/libm-test-ulps: Update.

Could you resend everything with the above changes, please? I'll commit 
then on your behalf...

Thanks a lot!
Andreas

> --
> Liubov Dmitrieva
> Intel Corporation
> 
> 2012/8/16 Dmitrieva Liubov <liubov.dmitrieva@gmail.com>:
> > False alarm. Our functions work correctly.
> > 
> > But anyway I have a separate patch for adding that test cases to
> > "make check" (attached).
> > It does not reveal new fails in current GLIBC, but several 1-ulp new
> > errors on IA.
> > And it does not reveal new fails in our new sinf/cosf  functions
> > (and
> > no 1-ulp new errors)
> > 
> > 
> > 2012-08-16  Liubov Dmitrieva  <liubov.dmitrieva@gmail.com>
> > 
> >          * math/libm-test.inc: Update
> >          Add new test cases in large arguments path.
> > 
> > So, no need to fix here, both patches are ok.
> > 
> > The latest version were attached to:
> > http://sourceware.org/ml/libc-alpha/2012-08/msg00267.html
> > http://sourceware.org/ml/libc-alpha/2012-08/msg00265.html
> > 
> > --
> > Liubov Dmitrieva
> > 
> > 2012/8/15 Joseph S. Myers <joseph@codesourcery.com>:
> >> On Wed, 15 Aug 2012, Dmitrieva Liubov wrote:
> >>> > This code is wrong. You cannot perform argument reduction for
> >>> > large
> >>> > arguments by using a single, double, or even extended-precision
> >>> > approximation for pi.
> >>> 
> >>> Yes, that's wrong in x86_32 version and will be fixed but 64 bit
> >>> version looks ok.
> >> 
> >> If that didn't get detected by the testsuite, I suppose we should
> >> add
> >> 0x1p+120 (or some such value that detects the problem) to the tests
> >> for cos and sin in libm-test.inc.  (The larest float value for cos
> >> in the testsuite is 0x1p65; sin also tests 0x1.7f4134p+103.)
> >> 
> >> --
> >> Joseph S. Myers
> >> joseph@codesourcery.com
-- 
 Andreas Jaeger aj@{suse.com,opensuse.org} Twitter/Identica: jaegerandi
  SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
   GF: Jeff Hawn,Jennifer Guild,Felix Imendörffer,HRB16746 (AG Nürnberg)
    GPG fingerprint = 93A3 365E CE47 B889 DF7F  FED1 389A 563C C272 A126