This is the mail archive of the
mailing list for the GCC project.
Re: patch to bug #86829
- From: Richard Biener <richard dot guenther at gmail dot com>
- To: Jeff Law <law at redhat dot com>
- Cc: giuliano dot belinassi at usp dot br, GCC Patches <gcc-patches at gcc dot gnu dot org>
- Date: Tue, 21 Aug 2018 10:02:57 +0200
- Subject: Re: patch to bug #86829
- References: <CAEFO=4AiqFvHH5sb1xWguEoSY2osH+NZJzWELkHqefEbgTd_6g@mail.gmail.com> <email@example.com>
On Mon, Aug 20, 2018 at 9:40 PM Jeff Law <firstname.lastname@example.org> wrote:
> On 08/04/2018 07:22 AM, Giuliano Augusto Faulin Belinassi wrote:
> > Closes bug #86829
> > Description: Adds substitution rules for both sin(atan(x)) and
> > cos(atan(x)). These formulas are replaced by x / sqrt(x*x + 1) and 1 /
> > sqrt(x*x + 1) respectively, providing up to 10x speedup. This identity
> > can be proved mathematically.
> > Changelog:
> > 2018-08-03 Giuliano Belinassi <email@example.com>
> > * match.pd: add simplification rules to sin(atan(x)) and cos(atan(x)).
> > Bootstrap and Testing:
> > There were no unexpected failures in a proper testing in GCC 8.1.0
> > under a x86_64 running Ubuntu 18.04.
> I understand these are mathematical identities. But floating point
> arthmetic in a compiler isn't nearly that clean :-) We have to worry
> about overflows, underflows, rounding, and the simple fact that many
> floating point numbers can't be exactly represented.
> Just as an example, compare the results for
> x = 0x1.fffffffffffffp1023
> I think sin(atan (x)) is well defined in that case. But the x*x isn't
> because it overflows.
> So I think this has to be somewhere under the -ffast-math umbrella.
> And the testing requirements for that are painful -- you have to verify
> it doesn't break the spec benchmark.
> I know Richi acked in the PR, but that might have been premature.
It's under the flag_unsafe_math_optimizations umbrella, but sure,
a "proper" way to optimize this would be to further expand
sqrt (x*x + 1) to fabs(x) + ... (extra terms) that are precise enough
and not have this overflow issue.
But yes, I do not find (quickly skimming) other simplifications that
have this kind of overflow issue (in fact I do remember raising
overflow/underflow issues for other patches).
Thus approval withdrawn.
If we had useful range info on floats we might conditionalize such
transforms appropriately. Or we can enable it on floats and do
the sqrt (x*x + 1) in double.