This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]