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

[Richard Biener via Gcc-patches]

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
>