Re: [PATCH1/2] Improve performance of sincosf

[Joseph Myers <joseph at codesourcery dot com>]

On Fri, 29 Jun 2018, Wilco Dijkstra wrote:

> 	* math/Makefile: Add s_sincosf_data.c.

You then need to add a dummy s_sincosf_data.c for architectures with their 
own sincosf implementation, to avoid wasting space in libm.so with unused 
data from s_sincosf_data.c.  That looks like ia64, m68k, x86_64.  (i686 
always builds in the generic version in case of a lack of SSE2 support, so 
no change is needed there, though I've no idea of the performance merits 
of the SSE2 version for i686 versus building the new generic one with SSE2 
enabled.)

> -/* Hack: only include the large arm_neon.h when needed.  */
> -#ifdef _MATH_CONFIG_H
> -# include <arm_neon.h>
> -
>  /* ACLE intrinsics for frintn and fcvtns instructions.  */
>  # define TOINT_INTRINSICS 1

Since you're removing the #ifdef you also need to remove the space between 
'#' and define (update the preprocessor indentation on what was 
conditional code).

>  #endif
>  
>  #include_next <math_private.h>
> -
> -#endif

So you seem to be moving the #include_next outside the multiple-include 
guards, so probably defeating the multiple-include optimization on this 
header.  Why?

> +#ifndef PREFER_FLOAT_COMPARISON
> +#define PREFER_FLOAT_COMPARISON 0
> +#endif

Missing preprocessor indentation, "# define".

> +/* The constants and polynomials for sine and cosine.  The 2nd entry
> +   computes -cos (x) rather than cos (x) to get negation for free.  */
> +const sincos_t sincosf_table[2] =

This should be __sincosf_table.  I'd have expected you to get 
linknamespace errors from testing your second patch with the name 
sincosf_table, which is in the user's namespace.  (They wouldn't have 
shown up with the first patch because sincosf isn't a standard function, 
but sinf and cosf are standard functions.)

-- 
Joseph S. Myers
joseph@codesourcery.com