[PATCH] Simplify sinh (x) / cosh (x)

[PATCH] Simplify sinh (x) / cosh (x)

[Rafael Tsuha]

[-- Attachment #1: Type: text/plain, Size: 712 bytes --]

This patch simplifies the expression sinh (x) / cosh (x) to tanh (x).
This rule is mathematically valid.

There's a slight difference in the result when applying this
optimization with x in the interval 0 < x <= 1e-4951. With the
optimization, the result using long double is -0 and without the
optimization, the result is +0.

When running the testsuite, the test gfortran.dg/pr79966.f90 failed,
but it seems unrelated to this patch

My architecture is x86_64.

gcc/ChangeLog
2019-01-22  Rafael Tsuha  <rafael.tsuha@usp.br>

        * match.pd (sinh (x) / cosh (x)): New simplification rule.

gcc/testsuite/ChangeLog
2019-01-22  Rafael Tsuha  <rafael.tsuha@usp.br>

        * gcc.dg/sinhovercosh-1.c: New test.

[-- Attachment #2: sinhovercosh.patch --]
[-- Type: text/x-patch, Size: 2103 bytes --]

Index: gcc/match.pd
===================================================================
--- gcc/match.pd	(revision 267671)
+++ gcc/match.pd	(working copy)
@@ -4484,6 +4484,11 @@
   (rdiv (SIN:s @0) (COS:s @0))
    (TAN @0))
 
+ /* Simplify sinh(x) / cosh(x) -> tanh(x). */
+ (simplify
+  (rdiv (SINH:s @0) (COSH:s @0))
+   (TANH @0))
+
  /* Simplify cos(x) / sin(x) -> 1 / tan(x). */
  (simplify
   (rdiv (COS:s @0) (SIN:s @0))
Index: gcc/testsuite/gcc.dg/sinhovercosh-1.c
===================================================================
--- gcc/testsuite/gcc.dg/sinhovercosh-1.c	(nonexistent)
+++ gcc/testsuite/gcc.dg/sinhovercosh-1.c	(working copy)
@@ -0,0 +1,45 @@
+/* { dg-do compile } */
+/* { dg-options "-Ofast -fdump-tree-optimized" } */
+
+extern float sinhf (float);
+extern float coshf (float);
+extern float tanhf (float);
+extern float sqrtf (float);
+extern double sinh (double);
+extern double cosh (double);
+extern double sqrt (double);
+extern double tanh (double);
+extern long double sinhl (long double);
+extern long double coshl (long double);
+extern long double tanhl (long double);
+extern long double sqrtl (long double);
+
+double __attribute__ ((noinline))
+sinhovercosh_ (double x)
+{
+  return sinh (x) / cosh (x);
+}
+
+float __attribute__ ((noinline))
+sinhfovercoshf_(float x)
+{
+  return sinhf (x) / coshf (x);
+}
+
+long double __attribute__ ((noinline))
+sinhlovercoshl_ (long double x)
+{
+  return sinhl (x) / coshl (x);
+}
+
+/* There must be no calls to sinh, cosh, or atanh */
+/* {dg-final { scan-tree-dump-not "sinh " "optimized" } } */
+/* {dg-final { scan-tree-dump-not "cosh " "optimized" } } */
+/* {dg-final { scan-tree-dump-not "sinfh " "optimized" } } */
+/* {dg-final { scan-tree-dump-not "cosfh " "optimized" } } */
+/* {dg-final { scan-tree-dump-not "sinlh " "optimized" } } */
+/* {dg-final { scan-tree-dump-not "coslh " "optimized" } } */
+/* {dg-final { scan-tree-dump-times "tanh " "1" "optimized" }} */
+/* {dg-final { scan-tree-dump-times "tanhl " "1" "optimized" }} */
+/* {dg-final { scan-tree-dump-times "tanhf " "1" "optimized" }} */
+

Re: [PATCH] Simplify sinh (x) / cosh (x)

[Jeff Law]

On 1/22/19 12:31 PM, Rafael Tsuha wrote:
> This patch simplifies the expression sinh (x) / cosh (x) to tanh (x).
> This rule is mathematically valid.
> 
> There's a slight difference in the result when applying this
> optimization with x in the interval 0 < x <= 1e-4951. With the
> optimization, the result using long double is -0 and without the
> optimization, the result is +0.
That's an indication something has gone wrong.

If I'm reading this correctly it sounds like tanh in that range is
returning -0?  If so, that just seems like the wrong output from tanh
since IIUC for a positive input tanh will always have a positive output.

Jeff

Re: [PATCH] Simplify sinh (x) / cosh (x)

[Rafael Tsuha]

Hi, Jeff

Em seg, 29 de abr de 2019 às 18:22, Jeff Law <law@redhat.com> escreveu:
>
> On 1/22/19 12:31 PM, Rafael Tsuha wrote:
> > This patch simplifies the expression sinh (x) / cosh (x) to tanh (x).
> > This rule is mathematically valid.
> >
> > There's a slight difference in the result when applying this
> > optimization with x in the interval 0 < x <= 1e-4951. With the
> > optimization, the result using long double is -0 and without the
> > optimization, the result is +0.
> That's an indication something has gone wrong.
>
> If I'm reading this correctly it sounds like tanh in that range is
> returning -0?  If so, that just seems like the wrong output from tanh
> since IIUC for a positive input tanh will always have a positive output.
>

I reverted the patch sent to solve bug 88556 and found out that
tanhl(0) started returning -0 after this patch.

patch we reverted:
(https://gcc.gnu.org/viewcvs/gcc/trunk/gcc/config/i386/i386.c?r1=267325&r2=267324&pathrev=267325)

In the line 44480 of this patch, it checks the sign bit of the input
and if it's false the expression is multiplied by -1. In the way it's
being calculated, this should be done only if the input is a number
greater than zero.

If we follow the code starting at line 44468, replacing op1 with 0, we
can see that e2 equals 0 at line 44482, flags will be false and
finally the e2 = -e2 operation will be executed generating the -0
result.

I have implemented a testcase to reproduce the bug:
https://paste.debian.net/1098800/
this code was compiled with -Ofast when we tested it.

Should I file a bug about this? And for fixing, Is it a good solution
to emit an instruction to return zero immediately if the input equals
zero?

> Jeff
>

Rafael Tsuha

Re: [PATCH] Simplify sinh (x) / cosh (x)

[Jeff Law]

On 9/4/19 12:16 PM, Rafael Tsuha wrote:
> Hi, Jeff
> 
> Em seg, 29 de abr de 2019 às 18:22, Jeff Law <law@redhat.com> escreveu:
>>
>> On 1/22/19 12:31 PM, Rafael Tsuha wrote:
>>> This patch simplifies the expression sinh (x) / cosh (x) to tanh (x).
>>> This rule is mathematically valid.
>>>
>>> There's a slight difference in the result when applying this
>>> optimization with x in the interval 0 < x <= 1e-4951. With the
>>> optimization, the result using long double is -0 and without the
>>> optimization, the result is +0.
>> That's an indication something has gone wrong.
>>
>> If I'm reading this correctly it sounds like tanh in that range is
>> returning -0?  If so, that just seems like the wrong output from tanh
>> since IIUC for a positive input tanh will always have a positive output.
>>
> 
> I reverted the patch sent to solve bug 88556 and found out that
> tanhl(0) started returning -0 after this patch.
> 
> patch we reverted:
> (https://gcc.gnu.org/viewcvs/gcc/trunk/gcc/config/i386/i386.c?r1=267325&r2=267324&pathrev=267325)
> 
> In the line 44480 of this patch, it checks the sign bit of the input
> and if it's false the expression is multiplied by -1. In the way it's
> being calculated, this should be done only if the input is a number
> greater than zero.
> 
> If we follow the code starting at line 44468, replacing op1 with 0, we
> can see that e2 equals 0 at line 44482, flags will be false and
> finally the e2 = -e2 operation will be executed generating the -0
> result.
> 
> I have implemented a testcase to reproduce the bug:
> https://paste.debian.net/1098800/
> this code was compiled with -Ofast when we tested it.
> 
> Should I file a bug about this? And for fixing, Is it a good solution
> to emit an instruction to return zero immediately if the input equals
> zero?
So if I'm understanding Uros's patch correctly, it's supposed to only be
used for -ffast-math where we don't necessarily honor signed zeros.

Are you applying the sinh/cosh -> tanh transformation only with
-ffast-math (it's been so long I simply can't remember).

jeff

Re: [PATCH] Simplify sinh (x) / cosh (x)

[Uros Bizjak]

On Mon, Sep 9, 2019 at 8:44 PM Jeff Law <law@redhat.com> wrote:
>
> On 9/4/19 12:16 PM, Rafael Tsuha wrote:
> > Hi, Jeff
> >
> > Em seg, 29 de abr de 2019 às 18:22, Jeff Law <law@redhat.com> escreveu:
> >>
> >> On 1/22/19 12:31 PM, Rafael Tsuha wrote:
> >>> This patch simplifies the expression sinh (x) / cosh (x) to tanh (x).
> >>> This rule is mathematically valid.
> >>>
> >>> There's a slight difference in the result when applying this
> >>> optimization with x in the interval 0 < x <= 1e-4951. With the
> >>> optimization, the result using long double is -0 and without the
> >>> optimization, the result is +0.
> >> That's an indication something has gone wrong.
> >>
> >> If I'm reading this correctly it sounds like tanh in that range is
> >> returning -0?  If so, that just seems like the wrong output from tanh
> >> since IIUC for a positive input tanh will always have a positive output.
> >>
> >
> > I reverted the patch sent to solve bug 88556 and found out that
> > tanhl(0) started returning -0 after this patch.
> >
> > patch we reverted:
> > (https://gcc.gnu.org/viewcvs/gcc/trunk/gcc/config/i386/i386.c?r1=267325&r2=267324&pathrev=267325)
> >
> > In the line 44480 of this patch, it checks the sign bit of the input
> > and if it's false the expression is multiplied by -1. In the way it's
> > being calculated, this should be done only if the input is a number
> > greater than zero.
> >
> > If we follow the code starting at line 44468, replacing op1 with 0, we
> > can see that e2 equals 0 at line 44482, flags will be false and
> > finally the e2 = -e2 operation will be executed generating the -0
> > result.
> >
> > I have implemented a testcase to reproduce the bug:
> > https://paste.debian.net/1098800/
> > this code was compiled with -Ofast when we tested it.
> >
> > Should I file a bug about this? And for fixing, Is it a good solution
> > to emit an instruction to return zero immediately if the input equals
> > zero?
> So if I'm understanding Uros's patch correctly, it's supposed to only be
> used for -ffast-math where we don't necessarily honor signed zeros.

True. The full patch is at [1], where it is evident that all these
expanders are protected by flag_unsafe_math_optimizations. As
explained in the patch sumbission, the equations are ported from [2],
so barring some unwanted bug in the porting, they should be equal. I
didn't analyse the correctness of the original equations.

> Are you applying the sinh/cosh -> tanh transformation only with
> -ffast-math (it's been so long I simply can't remember).

[1] https://gcc.gnu.org/ml/gcc-patches/2018-12/msg01447.html
[2] https://sourceware.org/ml/libc-alpha/2018-12/msg00772.html

Uros.

Re: [PATCH] Simplify sinh (x) / cosh (x)

[Jeff Law]

On 9/10/19 1:36 AM, Uros Bizjak wrote:
> On Mon, Sep 9, 2019 at 8:44 PM Jeff Law <law@redhat.com> wrote:
>>
>> On 9/4/19 12:16 PM, Rafael Tsuha wrote:
>>> Hi, Jeff
>>>
>>> Em seg, 29 de abr de 2019 às 18:22, Jeff Law <law@redhat.com> escreveu:
>>>>
>>>> On 1/22/19 12:31 PM, Rafael Tsuha wrote:
>>>>> This patch simplifies the expression sinh (x) / cosh (x) to tanh (x).
>>>>> This rule is mathematically valid.
>>>>>
>>>>> There's a slight difference in the result when applying this
>>>>> optimization with x in the interval 0 < x <= 1e-4951. With the
>>>>> optimization, the result using long double is -0 and without the
>>>>> optimization, the result is +0.
>>>> That's an indication something has gone wrong.
>>>>
>>>> If I'm reading this correctly it sounds like tanh in that range is
>>>> returning -0?  If so, that just seems like the wrong output from tanh
>>>> since IIUC for a positive input tanh will always have a positive output.
>>>>
>>>
>>> I reverted the patch sent to solve bug 88556 and found out that
>>> tanhl(0) started returning -0 after this patch.
>>>
>>> patch we reverted:
>>> (https://gcc.gnu.org/viewcvs/gcc/trunk/gcc/config/i386/i386.c?r1=267325&r2=267324&pathrev=267325)
>>>
>>> In the line 44480 of this patch, it checks the sign bit of the input
>>> and if it's false the expression is multiplied by -1. In the way it's
>>> being calculated, this should be done only if the input is a number
>>> greater than zero.
>>>
>>> If we follow the code starting at line 44468, replacing op1 with 0, we
>>> can see that e2 equals 0 at line 44482, flags will be false and
>>> finally the e2 = -e2 operation will be executed generating the -0
>>> result.
>>>
>>> I have implemented a testcase to reproduce the bug:
>>> https://paste.debian.net/1098800/
>>> this code was compiled with -Ofast when we tested it.
>>>
>>> Should I file a bug about this? And for fixing, Is it a good solution
>>> to emit an instruction to return zero immediately if the input equals
>>> zero?
>> So if I'm understanding Uros's patch correctly, it's supposed to only be
>> used for -ffast-math where we don't necessarily honor signed zeros.
> 
> True. The full patch is at [1], where it is evident that all these
> expanders are protected by flag_unsafe_math_optimizations. As
> explained in the patch sumbission, the equations are ported from [2],
> so barring some unwanted bug in the porting, they should be equal. I
> didn't analyse the correctness of the original equations.
It (your patch) looked fine to me given the -ffast-math constraint.

I think the question we need to go back and answer is why the proposed
patch to improve sinh/cosh -> tanh is using those expanders in an
unexpected way.

jeff

Re: [PATCH] Simplify sinh (x) / cosh (x)

[Rafael Tsuha]

Hi Jeff,

I've just checked and the proposed optimization is under
-funsafe-math-optimizations and canonicalize_math_p():
 `(if (flag_unsafe_math_optimizations && canonicalize_math_p ())`

I'm sorry, but I don't quite understand what you mean with 'using
those expanders in an unexpected way', should I put the optimization
in another place and try to deal with the zero sign or is it fine the
way it is?

Rafael Tsuha


Em ter, 10 de set de 2019 às 12:23, Jeff Law <law@redhat.com> escreveu:
>
> On 9/10/19 1:36 AM, Uros Bizjak wrote:
> > On Mon, Sep 9, 2019 at 8:44 PM Jeff Law <law@redhat.com> wrote:
> >>
> >> On 9/4/19 12:16 PM, Rafael Tsuha wrote:
> >>> Hi, Jeff
> >>>
> >>> Em seg, 29 de abr de 2019 às 18:22, Jeff Law <law@redhat.com> escreveu:
> >>>>
> >>>> On 1/22/19 12:31 PM, Rafael Tsuha wrote:
> >>>>> This patch simplifies the expression sinh (x) / cosh (x) to tanh (x).
> >>>>> This rule is mathematically valid.
> >>>>>
> >>>>> There's a slight difference in the result when applying this
> >>>>> optimization with x in the interval 0 < x <= 1e-4951. With the
> >>>>> optimization, the result using long double is -0 and without the
> >>>>> optimization, the result is +0.
> >>>> That's an indication something has gone wrong.
> >>>>
> >>>> If I'm reading this correctly it sounds like tanh in that range is
> >>>> returning -0?  If so, that just seems like the wrong output from tanh
> >>>> since IIUC for a positive input tanh will always have a positive output.
> >>>>
> >>>
> >>> I reverted the patch sent to solve bug 88556 and found out that
> >>> tanhl(0) started returning -0 after this patch.
> >>>
> >>> patch we reverted:
> >>> (https://gcc.gnu.org/viewcvs/gcc/trunk/gcc/config/i386/i386.c?r1=267325&r2=267324&pathrev=267325)
> >>>
> >>> In the line 44480 of this patch, it checks the sign bit of the input
> >>> and if it's false the expression is multiplied by -1. In the way it's
> >>> being calculated, this should be done only if the input is a number
> >>> greater than zero.
> >>>
> >>> If we follow the code starting at line 44468, replacing op1 with 0, we
> >>> can see that e2 equals 0 at line 44482, flags will be false and
> >>> finally the e2 = -e2 operation will be executed generating the -0
> >>> result.
> >>>
> >>> I have implemented a testcase to reproduce the bug:
> >>> https://paste.debian.net/1098800/
> >>> this code was compiled with -Ofast when we tested it.
> >>>
> >>> Should I file a bug about this? And for fixing, Is it a good solution
> >>> to emit an instruction to return zero immediately if the input equals
> >>> zero?
> >> So if I'm understanding Uros's patch correctly, it's supposed to only be
> >> used for -ffast-math where we don't necessarily honor signed zeros.
> >
> > True. The full patch is at [1], where it is evident that all these
> > expanders are protected by flag_unsafe_math_optimizations. As
> > explained in the patch sumbission, the equations are ported from [2],
> > so barring some unwanted bug in the porting, they should be equal. I
> > didn't analyse the correctness of the original equations.
> It (your patch) looked fine to me given the -ffast-math constraint.
>
> I think the question we need to go back and answer is why the proposed
> patch to improve sinh/cosh -> tanh is using those expanders in an
> unexpected way.
>
> jeff

Re: [PATCH] Simplify sinh (x) / cosh (x)

[Jeff Law]

On 10/2/19 12:08 PM, Rafael Tsuha wrote:
> Hi Jeff,
> 
> I've just checked and the proposed optimization is under
> -funsafe-math-optimizations and canonicalize_math_p():
>  `(if (flag_unsafe_math_optimizations && canonicalize_math_p ())`
> 
> I'm sorry, but I don't quite understand what you mean with 'using
> those expanders in an unexpected way', should I put the optimization
> in another place and try to deal with the zero sign or is it fine the
> way it is?
So I went back to the original discussion to try and get context.

In that discussion you introduced this pattern for match.pd:

> + /* Simplify sinh(x) / cosh(x) -> tanh(x). */
> + (simplify
> +  (rdiv (SINH:s @0) (COSH:s @0))
> +   (TANH @0))

You indicated that it was giving unexpected results in a particular
interval:

> There's a slight difference in the result when applying this
> optimization with x in the interval 0 < x <= 1e-4951. With the
> optimization, the result using long double is -0 and without the
> optimization, the result is +0.

That was worrisome (and it wasn't clear from the patch that this was
predicated on flag_unsafe_math_optimizations).  So at some point I
speculated there was a deeper problem, perhaps in the tanh support.  I
think I sent you down a rathole -- sorry.

In fact, your pattern is predicated on flag_unsafe_math_optimizations
which, among other things, means that we don't honor signed zeros.  That
also explains how we were able to get into Uros's code which is also
predicated on not honoring signed zeros.

When we are not honoring signed zeros, changes like you observed are
undesirable, but OK.

I'll install your patch momentarily.  THanks for your patience,


Jeff