diff mbox series

A == 0 ? A : -A same as -A (when A is 0.0)

Message ID 20220829204439.1749727-1-aldyh@redhat.com
State New
Headers
Series A == 0 ? A : -A same as -A (when A is 0.0) |

Commit Message

Aldy Hernandez Aug. 29, 2022, 8:44 p.m. UTC 
  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(+)

Comments

Richard Biener Aug. 30, 2022, 7:03 a.m. UTC | #1 
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).

OK if doing it that way.

Thanks,
Richard.

> --
> 2.37.1
>
Aldy Hernandez Aug. 30, 2022, 7:06 a.m. UTC | #2 
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
> >
>
Richard Biener Aug. 30, 2022, 7:12 a.m. UTC | #3 
On Tue, Aug 30, 2022 at 9:07 AM Aldy Hernandez <aldyh@redhat.com> wrote:
>
> 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.

Well, it already applies when the constant is not propagated, so ...

> Thanks.
> Aldy
>
> >
> > OK if doing it that way.
> >
> > Thanks,
> > Richard.
> >
> > > --
> > > 2.37.1
> > >
> >
>
Jeff Law Sept. 7, 2022, 4:54 p.m. UTC | #4 
On 8/29/2022 2:44 PM, Aldy Hernandez via Gcc-patches 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.
OK
jeff
diff mbox series

Patch

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))