Re: [PATCH] A == 0 ? A : -A same as -A (when A is 0.0)

[Aldy Hernandez <aldyh@redhat.com>]

On Tue, Aug 30, 2022 at 9:03 AM Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On Mon, Aug 29, 2022 at 10:45 PM Aldy Hernandez via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
> >
> > The upcoming work for frange triggers a regression in
> > gcc.dg/tree-ssa/phi-opt-24.c.
> >
> > For -O2 -fno-signed-zeros, we fail to transform the following into -A:
> >
> > float f0(float A)
> > {
> >   //     A == 0? A : -A    same as -A
> >   if (A == 0)  return A;
> >   return -A;
> > }
> >
> > This is because the abs/negative match.pd pattern here:
> >
> > /* abs/negative simplifications moved from fold_cond_expr_with_comparison,
> >    Need to handle (A - B) case as fold_cond_expr_with_comparison does.
> >    Need to handle UN* comparisons.
> >    ...
> >    ...
> >
> > Sees IL that has the 0.0 propagated.
> >
> > Instead of:
> >
> >   <bb 2> [local count: 1073741824]:
> >   if (A_2(D) == 0.0)
> >     goto <bb 4>; [34.00%]
> >   else
> >     goto <bb 3>; [66.00%]
> >
> >   <bb 3> [local count: 708669601]:
> >   _3 = -A_2(D);
> >
> >   <bb 4> [local count: 1073741824]:
> >   # _1 = PHI <A_2(D)(2), _3(3)>
> >
> > It now sees:
> >
> >   <bb 4> [local count: 1073741824]:
> >   # _1 = PHI <0.0(2), _3(3)>
> >
> > which it leaves untouched, causing the if conditional to survive.
> >
> > Adding a variant to the pattern that for real_zerop fixes the problem.
> >
> > I am a match.pd weenie, and am avoiding touching more patterns that
> > may also benefit from handling real constants.  So please use simple
> > words if what I'm doing isn't correct ;-).
> >
> > I did not include a testcase, as it's just phi-opt-24.c which will get
> > triggered when I commit the frange with endpoints work.
> >
> > Tested on x86-64 & ppc64le Linux.
> >
> > OK?
> >
> > gcc/ChangeLog:
> >
> >         * match.pd ((cmp @0 zerop) real_zerop (negate@1 @0)): Add variant
> >         for real zero.
> > ---
> >  gcc/match.pd | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/gcc/match.pd b/gcc/match.pd
> > index 1bb936fc401..5bdea300f94 100644
> > --- a/gcc/match.pd
> > +++ b/gcc/match.pd
> > @@ -4803,6 +4803,10 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
> >     (cnd (cmp @0 zerop) @0 (negate@1 @0))
> >      (if (!HONOR_SIGNED_ZEROS (type))
> >       @1))
> > +  (simplify
> > +   (cnd (cmp @0 zerop) real_zerop (negate@1 @0))
> > +    (if (!HONOR_SIGNED_ZEROS (type))
> > +     @1))
> >    (simplify
> >     (cnd (cmp @0 zerop) integer_zerop (negate@1 @0))
> >      (if (!HONOR_SIGNED_ZEROS (type))
>
> Looking at zerop:
>
> bool
> zerop (const_tree expr)
> {
>   return (integer_zerop (expr)
>           || real_zerop (expr)
>           || fixed_zerop (expr));
> }
>
> your patch is equivalent to changing the integer_zerop one to use zerop?
> (well, with the exception of also covering fixed_zerop).

Heh.  That was my first instinct, but I thought fixed_zerop didn't
apply?  No clue...I'm happy to change it.

Thanks.
Aldy

>
> OK if doing it that way.
>
> Thanks,
> Richard.
>
> > --
> > 2.37.1
> >
>