| Message ID | CAEFO=4DeuEG01fxQ=tkATC53MT8SXzsJDfOdCuu2Xv=DPSuGvA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] Add sinh(atanh(x)) and cosh(atanh(x)) optimizations | expand |
On Tue, Nov 13, 2018 at 10:25 PM Giuliano Augusto Faulin Belinassi <giuliano.belinassi@usp.br> wrote: > > Only do the optimization if flag_signed_zeros && > !flag_finite_math_only is set, as suggested in the previous iteration. > > Before, the patch did the optimization even when -fno-signed-zeros and > -ffinite-math-only was set. This could generate badly incorrect > results for targets that do not support infinite or signed zeros. How's the result wrong if there are no signed zeros? Note that -ffast-math enables -fno-signed-zeros for example. So both of your check look backwards. Also the support for signed zeros and infs/nans should be guarded with !HONOR_SIGNED_ZEROS (type) && !HONOR_NANS (type) && !HONOR_INFINITIES (type) which then means there's no difference between -0. and 0. and there are no NaNs or Infs in the inputs and ouptut NaNs or Infs need not be produced. Richard. > I also updated the tests with the proper flags. > > gcc/ChangeLog > 2018-11-13 Giuliano Belinassi <giuliano.belinassi@usp.br> > > * match.pd (sinh (atanh (x))): New simplification rules. > (cosh (atanh (x))): Likewise. > > gcc/testsuite/ChangeLog > 2018-11-13 Giuliano Belinassi <giuliano.belinassi@usp.br> > > * gcc.dg/sinhatanh-1.c: New test. > * gcc.dg/sinhatanh-2.c: New test. > > There are no tests in trunk that seems to be breaking because of this patch.
On Wed, Nov 14, 2018 at 7:41 AM Richard Biener <richard.guenther@gmail.com> wrote: > > On Tue, Nov 13, 2018 at 10:25 PM Giuliano Augusto Faulin Belinassi > <giuliano.belinassi@usp.br> wrote: > > > > Only do the optimization if flag_signed_zeros && > > !flag_finite_math_only is set, as suggested in the previous iteration. > > > > Before, the patch did the optimization even when -fno-signed-zeros and > > -ffinite-math-only was set. This could generate badly incorrect > > results for targets that do not support infinite or signed zeros. > > How's the result wrong if there are no signed zeros? Note that -ffast-math > enables -fno-signed-zeros for example. So both of your check look > backwards. Indeed. After plotting the graph of both functions, it is very clear that this check isn't required. Sorry about that. > Also the support for signed zeros and infs/nans should be guarded with > > !HONOR_SIGNED_ZEROS (type) && !HONOR_NANS (type) && !HONOR_INFINITIES (type) > > which then means there's no difference between -0. and 0. and there are no > NaNs or Infs in the inputs and ouptut NaNs or Infs need not be produced. There can be NaNs and Infinities. For NaNs, take any input that is outside the [-1, 1] line. For Infinities, take x = -1, or x = 1. I think these must be 'honored' as to ensure compatibility with the original expression. so I must check for !HONOR_SIGNED_ZEROS (type) && HONOR_NANS (type) && HONOR_INFINITIES (type) that is correct? Also, is it safe to remove the !finite_math_only with this, as now it is stated that the type supports infinity and NaNs? However, I am not sure if it is OK to remove unsafe-math-optimizations even if it enables finite_math_only because of the 2 ULP error. As stated in the first iteration, the user can be using a very precise math library that yields 0 ULP. > Richard. > > > I also updated the tests with the proper flags. > > > > gcc/ChangeLog > > 2018-11-13 Giuliano Belinassi <giuliano.belinassi@usp.br> > > > > * match.pd (sinh (atanh (x))): New simplification rules. > > (cosh (atanh (x))): Likewise. > > > > gcc/testsuite/ChangeLog > > 2018-11-13 Giuliano Belinassi <giuliano.belinassi@usp.br> > > > > * gcc.dg/sinhatanh-1.c: New test. > > * gcc.dg/sinhatanh-2.c: New test. > > > > There are no tests in trunk that seems to be breaking because of this patch.
Hi, > Indeed. After plotting the graph of both functions, it is very clear > that this check isn't required. Sorry about that. It wouldn't be clear from the graph, you need to check that +0.0, -0.0, out of range values, infinities, NaNs give the same answer before/after your transformation. If so, then you don't need anything extra except for unsafe-math-optimizations and no-math-errno (given errno handling is changed). > There can be NaNs and Infinities. For NaNs, take any input that is > outside the [-1, 1] line. > For Infinities, take x = -1, or x = 1. I think these must be 'honored' > as to ensure compatibility with the original expression. The question is whether you get the same answer for these, not whether you can end up with an infinity or NaN. The idea is that we optimize based on the assumption there are no infinities or NaNs. FP operations can still produce infinities or NaNs, the compiler just doesn't need to worry about treating them correctly, and it's the programmer's reponsibility to ensure they are not generated. > so I must check for > !HONOR_SIGNED_ZEROS (type) && HONOR_NANS (type) && HONOR_INFINITIES (type) > that is correct? Also, is it safe to remove the !finite_math_only with > this, as now it is stated that the type supports infinity and NaNs? No that doesn't look quite right. First check whether the transformation handles zero/inf/NaN correctly, if so you don't need any of this. > However, I am not sure if it is OK to remove unsafe-math-optimizations > even if it enables > finite_math_only because of the 2 ULP error. As stated in the first > iteration, the user can be > using a very precise math library that yields 0 ULP. Well 0 ULP would be an impossibility. Unsafe math seems reasonable since it does behave slightly differently (including in terms of exception flags set). It's unfortunate GCC doesn't have clear definition of IEEE conformance modes like various other compilers. Wilco
Hi, Sorry, but I am a little confused. On Wed, Nov 14, 2018 at 11:28 AM Wilco Dijkstra <Wilco.Dijkstra@arm.com> wrote: > > Hi, > > > > Indeed. After plotting the graph of both functions, it is very clear > > that this check isn't required. Sorry about that. > > It wouldn't be clear from the graph, you need to check that +0.0, -0.0, > out of range values, infinities, NaNs give the same answer before/after > your transformation. If so, then you don't need anything extra except > for unsafe-math-optimizations and no-math-errno (given errno handling > is changed). I checked it. They are all the same on x86_64: https://pastebin.com/e63FxDAy I even forced to call the glibc sinh and atanh, but use the sqrtsd instruction. But I do agree that there may be an arch that sets an errno for sinh or cosh but not for sqrt, implying in a unexpected behavior. Is the no-math-errno really necessary? > > > There can be NaNs and Infinities. For NaNs, take any input that is > > outside the [-1, 1] line. > > For Infinities, take x = -1, or x = 1. I think these must be 'honored' > > as to ensure compatibility with the original expression. > > The question is whether you get the same answer for these, not whether > you can end up with an infinity or NaN. The idea is that we optimize based > on the assumption there are no infinities or NaNs. FP operations can still > produce infinities or NaNs, the compiler just doesn't need to worry about > treating them correctly, and it's the programmer's reponsibility to ensure > they are not generated. > > > so I must check for > > !HONOR_SIGNED_ZEROS (type) && HONOR_NANS (type) && HONOR_INFINITIES (type) > > that is correct? Also, is it safe to remove the !finite_math_only with > > this, as now it is stated that the type supports infinity and NaNs? > > No that doesn't look quite right. First check whether the transformation > handles zero/inf/NaN correctly, if so you don't need any of this. > It does handle these correctly, as far as I am concerned. At least in IEEE 754. Richard Biener asked to add !flag_siged_zeros for documentation reasons but this is already covered by unsafe-math. I also checked the flags from the optimization page, but only unsafe math seems to be applicable. https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html So maybe just check for unsafe-math? > > > However, I am not sure if it is OK to remove unsafe-math-optimizations > > even if it enables > > finite_math_only because of the 2 ULP error. As stated in the first > > iteration, the user can be > > using a very precise math library that yields 0 ULP. > > Well 0 ULP would be an impossibility. Unsafe math seems reasonable since > it does behave slightly differently (including in terms of exception flags set). > It's unfortunate GCC doesn't have clear definition of IEEE conformance > modes like various other compilers. > > Wilco > > >
Hi, > I checked it. They are all the same on x86_64: > https://pastebin.com/e63FxDAy > I even forced to call the glibc sinh and atanh, but use the sqrtsd instruction. > But I do agree that there may be an arch that sets an errno for sinh > or cosh but not for sqrt, implying in a unexpected behavior. > Is the no-math-errno really necessary? Thanks for the code, that was quite useful. When using errno it is actually necessary to reset errno before every math function since it otherwise contains an incorrect value from a different math call. When I do that I get: before: = -nan after : = -nan before: = -nan after : = -nan before: = nan after : = nan before: = -0.0000000000e+00 after : = -0.0000000000e+00 before: = 0.0000000000e+00 after : = 0.0000000000e+00 before: = inf after : = inf Errno changed 34 0 before: = nan after : = nan before: = nan after : = nan before: = nan after : = nan So NaNs, signed zeroes, infinities all work perfectly indeed, and it's just errno that isn't set in the same way for the infinite case. That means that checking for errno in addition to unsafe-math would be good, unless unsafe-math already allows incorrect setting of errno. Wilco
Index: gcc/match.pd =================================================================== --- gcc/match.pd (revision 266029) +++ gcc/match.pd (working copy) @@ -4311,6 +4311,26 @@ (rdiv { t_one; } (sqrts (plus (mult @0 @0) { t_one; }))) (copysigns { t_zero; } @0)))))) + /* Simplify sinh(atanh(x)) -> x / sqrt((1 - x)*(1 + x)). */ + (for sinhs (SINH) + atanhs (ATANH) + sqrts (SQRT) + (simplify + (sinhs (atanhs:s @0)) + (with { tree t_one = build_one_cst (type); } + (if (flag_signed_zeros && !flag_finite_math_only) + (rdiv @0 (sqrts (mult (minus { t_one; } @0) (plus { t_one; } @0)))))))) + + /* Simplify cosh(atanh(x)) -> 1 / sqrt((1 - x)*(1 + x)) */ + (for coshs (COSH) + atanhs (ATANH) + sqrts (SQRT) + (simplify + (coshs (atanhs:s @0)) + (with { tree t_one = build_real (type, dconst1); } + (if (flag_signed_zeros && !flag_finite_math_only) + (rdiv { t_one; } (sqrts (mult (minus { t_one; } @0) (plus { t_one; } @0)))))))) + /* cabs(x+0i) or cabs(0+xi) -> abs(x). */ (simplify (CABS (complex:C @0 real_zerop@1)) Index: gcc/testsuite/gcc.dg/sinhatanh-1.c =================================================================== --- gcc/testsuite/gcc.dg/sinhatanh-1.c (nonexistent) +++ gcc/testsuite/gcc.dg/sinhatanh-1.c (working copy) @@ -0,0 +1,64 @@ +/* { dg-do compile } */ +/* { dg-options "-Ofast -funsafe-math-optimizations -fsigned-zeros + -fno-finite-math-only -fno-associative-math + -fdump-tree-optimized" } */ + +extern float sinhf (float); +extern float coshf (float); +extern float atanhf (float); +extern float sqrtf (float); +extern double sinh (double); +extern double cosh (double); +extern double atanh (double); +extern double sqrt (double); +extern long double sinhl (long double); +extern long double coshl (long double); +extern long double atanhl (long double); +extern long double sqrtl (long double); + +double __attribute__ ((noinline)) +sinhatanh_ (double x) +{ + return sinh (atanh (x)); +} + +double __attribute__ ((noinline)) +coshatanh_ (double x) +{ + return cosh (atanh (x)); +} + +float __attribute__ ((noinline)) +sinhatanhf_(float x) +{ + return sinhf (atanhf (x)); +} + +float __attribute__ ((noinline)) +coshatanhf_(float x) +{ + return coshf (atanhf (x)); +} + +long double __attribute__ ((noinline)) +sinhatanhl_ (long double x) +{ + return sinhl (atanhl (x)); +} + +long double __attribute__ ((noinline)) +coshatanhl_ (long double x) +{ + return coshl (atanhl (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 "atanh " "optimized" }} */ +/* {dg-final { scan-tree-dump-not "sinfh " "optimized" } } */ +/* {dg-final { scan-tree-dump-not "cosfh " "optimized" } } */ +/* {dg-final { scan-tree-dump-not "atanfh " "optimized" }} */ +/* {dg-final { scan-tree-dump-not "sinlh " "optimized" } } */ +/* {dg-final { scan-tree-dump-not "coslh " "optimized" } } */ +/* {dg-final { scan-tree-dump-not "atanlh " "optimized" }} */ Index: gcc/testsuite/gcc.dg/sinhatanh-2.c =================================================================== --- gcc/testsuite/gcc.dg/sinhatanh-2.c (nonexistent) +++ gcc/testsuite/gcc.dg/sinhatanh-2.c (working copy) @@ -0,0 +1,70 @@ +/* { dg-do compile } */ +/* { dg-options "-Ofast -funsafe-math-optimizations -fsigned-zeros + -fno-finite-math-only -fno-associative-math + -fdump-tree-optimized" } */ + +extern float sinhf (float); +extern float coshf (float); +extern float atanhf (float); +extern float sqrtf (float); +extern double sinh (double); +extern double cosh (double); +extern double atanh (double); +extern double sqrt (double); +extern long double sinhl (long double); +extern long double coshl (long double); +extern long double atanhl (long double); +extern long double sqrtl (long double); + +float __attribute__ ((noinline)) +coshatanhf_(float x) +{ + float atg = atanhf(x); + return coshf(atg) + atg; +} + +double __attribute__ ((noinline)) +cosatan_(double x) +{ + double atg = atanh(x); + return cosh(atg) + atg; +} + +long double __attribute__ ((noinline)) +cosatanl_(long double x) +{ + long double atg = atanhl(x); + return coshl(atg) + atg; +} + +float __attribute__ ((noinline)) +sinatanf_(float x) +{ + float atg = atanhf(x); + return sinhf(atg) + atg; +} + +double __attribute__ ((noinline)) +sinatan_(double x) +{ + double atg = atanh(x); + return sinh(atg) + atg; +} + +long double __attribute__ ((noinline)) +sinatanl_(long double x) +{ + long double atg = atanhl(x); + return sinhl(atg) + atg; +} + +/* There should be calls to sinh, cosh and atanh */ +/* { dg-final { scan-tree-dump "cosh " "optimized" } } */ +/* { dg-final { scan-tree-dump "sinh " "optimized" } } */ +/* { dg-final { scan-tree-dump "atanh " "optimized" } } */ +/* { dg-final { scan-tree-dump "coshf " "optimized" } } */ +/* { dg-final { scan-tree-dump "sinhf " "optimized" } } */ +/* { dg-final { scan-tree-dump "atanhf " "optimized" } } */ +/* { dg-final { scan-tree-dump "coshl " "optimized" } } */ +/* { dg-final { scan-tree-dump "sinhl " "optimized" } } */ +/* { dg-final { scan-tree-dump "atanhl " "optimized" } } */
Only do the optimization if flag_signed_zeros && !flag_finite_math_only is set, as suggested in the previous iteration. Before, the patch did the optimization even when -fno-signed-zeros and -ffinite-math-only was set. This could generate badly incorrect results for targets that do not support infinite or signed zeros. I also updated the tests with the proper flags. gcc/ChangeLog 2018-11-13 Giuliano Belinassi <giuliano.belinassi@usp.br> * match.pd (sinh (atanh (x))): New simplification rules. (cosh (atanh (x))): Likewise. gcc/testsuite/ChangeLog 2018-11-13 Giuliano Belinassi <giuliano.belinassi@usp.br> * gcc.dg/sinhatanh-1.c: New test. * gcc.dg/sinhatanh-2.c: New test. There are no tests in trunk that seems to be breaking because of this patch.