From: Joseph Myers <joseph@codesourcery.com>
To: Wilco Dijkstra <Wilco.Dijkstra@arm.com>
Cc: "libc-alpha@sourceware.org" <libc-alpha@sourceware.org>, nd <nd@arm.com>
Subject: Re: [PATCH1/2] Improve performance of sincosf
Date: Fri, 29 Jun 2018 20:25:00 -0000 [thread overview]
Message-ID: <alpine.DEB.2.20.1806292016330.642@digraph.polyomino.org.uk> (raw)
In-Reply-To: <DB5PR08MB1030632EF77F675D8FE8FBF3834E0@DB5PR08MB1030.eurprd08.prod.outlook.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
next prev parent reply other threads:[~2018-06-29 20:25 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-06-26 11:18 Wilco Dijkstra
2018-06-28 17:24 ` Joseph Myers
2018-06-29 12:20 ` Wilco Dijkstra
2018-06-29 20:25 ` Joseph Myers [this message]
2018-06-30 14:49 ` Wilco Dijkstra
2018-06-30 20:25 ` Joseph Myers
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=alpine.DEB.2.20.1806292016330.642@digraph.polyomino.org.uk \
--to=joseph@codesourcery.com \
--cc=Wilco.Dijkstra@arm.com \
--cc=libc-alpha@sourceware.org \
--cc=nd@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).