* [PATCH] Simplify sinh (x) / cosh (x) @ 2019-01-22 19:31 Rafael Tsuha 2019-04-29 21:23 ` Jeff Law 0 siblings, 1 reply; 8+ messages in thread From: Rafael Tsuha @ 2019-01-22 19:31 UTC (permalink / raw) To: gcc-patches; +Cc: mateus.carmo.barbosa, giuliano.belinassi [-- 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" }} */ + ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Simplify sinh (x) / cosh (x) 2019-01-22 19:31 [PATCH] Simplify sinh (x) / cosh (x) Rafael Tsuha @ 2019-04-29 21:23 ` Jeff Law 2019-09-04 18:16 ` Rafael Tsuha 0 siblings, 1 reply; 8+ messages in thread From: Jeff Law @ 2019-04-29 21:23 UTC (permalink / raw) To: Rafael Tsuha, gcc-patches; +Cc: mateus.carmo.barbosa, giuliano.belinassi 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Simplify sinh (x) / cosh (x) 2019-04-29 21:23 ` Jeff Law @ 2019-09-04 18:16 ` Rafael Tsuha 2019-09-09 18:44 ` Jeff Law 0 siblings, 1 reply; 8+ messages in thread From: Rafael Tsuha @ 2019-09-04 18:16 UTC (permalink / raw) To: Jeff Law Cc: gcc-patches, Mateus Carmo Martins de Freitas Barbosa, Giuliano Augusto Faulin Belinassi 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Simplify sinh (x) / cosh (x) 2019-09-04 18:16 ` Rafael Tsuha @ 2019-09-09 18:44 ` Jeff Law 2019-09-10 7:37 ` Uros Bizjak 0 siblings, 1 reply; 8+ messages in thread From: Jeff Law @ 2019-09-09 18:44 UTC (permalink / raw) To: Rafael Tsuha Cc: gcc-patches, Mateus Carmo Martins de Freitas Barbosa, Giuliano Augusto Faulin Belinassi, Uros Bizjak 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Simplify sinh (x) / cosh (x) 2019-09-09 18:44 ` Jeff Law @ 2019-09-10 7:37 ` Uros Bizjak 2019-09-10 15:23 ` Jeff Law 0 siblings, 1 reply; 8+ messages in thread From: Uros Bizjak @ 2019-09-10 7:37 UTC (permalink / raw) To: Jeff Law Cc: Rafael Tsuha, gcc-patches, Mateus Carmo Martins de Freitas Barbosa, Giuliano Augusto Faulin Belinassi 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Simplify sinh (x) / cosh (x) 2019-09-10 7:37 ` Uros Bizjak @ 2019-09-10 15:23 ` Jeff Law 2019-10-02 18:08 ` Rafael Tsuha 0 siblings, 1 reply; 8+ messages in thread From: Jeff Law @ 2019-09-10 15:23 UTC (permalink / raw) To: Uros Bizjak Cc: Rafael Tsuha, gcc-patches, Mateus Carmo Martins de Freitas Barbosa, Giuliano Augusto Faulin Belinassi 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Simplify sinh (x) / cosh (x) 2019-09-10 15:23 ` Jeff Law @ 2019-10-02 18:08 ` Rafael Tsuha 2019-10-04 18:31 ` Jeff Law 0 siblings, 1 reply; 8+ messages in thread From: Rafael Tsuha @ 2019-10-02 18:08 UTC (permalink / raw) To: Jeff Law Cc: Uros Bizjak, gcc-patches, Mateus Carmo Martins de Freitas Barbosa, Giuliano Augusto Faulin Belinassi 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Simplify sinh (x) / cosh (x) 2019-10-02 18:08 ` Rafael Tsuha @ 2019-10-04 18:31 ` Jeff Law 0 siblings, 0 replies; 8+ messages in thread From: Jeff Law @ 2019-10-04 18:31 UTC (permalink / raw) To: Rafael Tsuha Cc: Uros Bizjak, gcc-patches, Mateus Carmo Martins de Freitas Barbosa, Giuliano Augusto Faulin Belinassi 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-10-04 18:31 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-01-22 19:31 [PATCH] Simplify sinh (x) / cosh (x) Rafael Tsuha 2019-04-29 21:23 ` Jeff Law 2019-09-04 18:16 ` Rafael Tsuha 2019-09-09 18:44 ` Jeff Law 2019-09-10 7:37 ` Uros Bizjak 2019-09-10 15:23 ` Jeff Law 2019-10-02 18:08 ` Rafael Tsuha 2019-10-04 18:31 ` Jeff Law
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).