Re: [PATCH] move the (a-b) CMP 0 ? (a-b) : (b-a) optimization from fold_cond_expr_with_comparison to match

[Richard Biener <richard.guenther@gmail.com>]

On Sun, Jul 11, 2021 at 4:12 AM apinski--- via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> From: Andrew Pinski <apinski@marvell.com>
>
> This patch moves the (a-b) CMP 0 ? (a-b) : (b-a) optimization
> from fold_cond_expr_with_comparison to match.

So I searched and I guess these transforms are produced from

  /* If we have A op 0 ? A : -A, consider applying the following
     transformations:

     A == 0? A : -A    same as -A
     A != 0? A : -A    same as A
     A >= 0? A : -A    same as abs (A)
     A > 0?  A : -A    same as abs (A)
     A <= 0? A : -A    same as -abs (A)
     A < 0?  A : -A    same as -abs (A)

     None of these transformations work for modes with signed
     zeros.  If A is +/-0, the first two transformations will
     change the sign of the result (from +0 to -0, or vice
     versa).  The last four will fix the sign of the result,
     even though the original expressions could be positive or
     negative, depending on the sign of A.

     Note that all these transformations are correct if A is
     NaN, since the two alternatives (A and -A) are also NaNs.  */
  if (!HONOR_SIGNED_ZEROS (type)
      && (FLOAT_TYPE_P (TREE_TYPE (arg01))
          ? real_zerop (arg01)
          : integer_zerop (arg01))
      && ((TREE_CODE (arg2) == NEGATE_EXPR
           && operand_equal_p (TREE_OPERAND (arg2, 0), arg1, 0))
             /* In the case that A is of the form X-Y, '-A' (arg2) may
                have already been folded to Y-X, check for that. */
          || (TREE_CODE (arg1) == MINUS_EXPR
              && TREE_CODE (arg2) == MINUS_EXPR
              && operand_equal_p (TREE_OPERAND (arg1, 0),
                                  TREE_OPERAND (arg2, 1), 0)
              && operand_equal_p (TREE_OPERAND (arg1, 1),
                                  TREE_OPERAND (arg2, 0), 0))))
...

I wonder at which point we can remove the code from fold-const.c?

Some comments inline below.

> OK? Bootstrapped and tested on x86_64-linux-gnu.
>
> gcc/ChangeLog:
>
>         * match.pd ((A-B) CMP 0 ? (A-B) : (B - A)):
>         New patterns.
>
> gcc/testsuite/ChangeLog:
>
>         * gcc.dg/tree-ssa/phi-opt-25.c: New test.
> ---
>  gcc/match.pd                               | 48 ++++++++++++++++++++--
>  gcc/testsuite/gcc.dg/tree-ssa/phi-opt-25.c | 45 ++++++++++++++++++++
>  2 files changed, 90 insertions(+), 3 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/phi-opt-25.c
>
> diff --git a/gcc/match.pd b/gcc/match.pd
> index 30680d488ab..aa88381fdcb 100644
> --- a/gcc/match.pd
> +++ b/gcc/match.pd
> @@ -4040,9 +4040,7 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
>    (cnd (logical_inverted_value truth_valued_p@0) @1 @2)
>    (cnd @0 @2 @1)))
>
> -/* 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.
> +/* abs/negative simplifications moved from fold_cond_expr_with_comparison.
>
>     None of these transformations work for modes with signed
>     zeros.  If A is +/-0, the first two transformations will
> @@ -4098,6 +4096,50 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
>         (convert (negate (absu:utype @0))))
>         (negate (abs @0)))))
>   )
> +
> + /* (A - B) == 0 ? (A - B) : (B - A)    same as (B - A) */
> + (for cmp (eq uneq)
> +  (simplify
> +   (cnd (cmp (minus@0 @1 @2) zerop) @0 (minus@3 @2 @1))
> +    (if (!HONOR_SIGNED_ZEROS (type))
> +     @3))
> +  (simplify
> +   (cnd (cmp (minus@0 @1 @2) zerop) integer_zerop (minus@3 @2 @1))

So that makes me think why integer_zerop?  'type' should then be
integer and thus never HONOR_SIGNED_ZEROS.

Don't we also need the inverted condition case for completeness?

> +    (if (!HONOR_SIGNED_ZEROS (type))
> +     @3))
> +  (simplify
> +   (cnd (cmp @1 @2) integer_zerop (minus@3 @2 @1))

I think this needs to be (cmp:c @1 @2)

> +    (if (!HONOR_SIGNED_ZEROS (type))
> +     @3))
> + )
> + /* (A - B) != 0 ? (A - B) : (B - A)    same as (A - B) */
> + (for cmp (ne ltgt)
> +  (simplify
> +   (cnd (cmp (minus@0 @1 @2) zerop) @0 (minus @2 @1))
> +    (if (!HONOR_SIGNED_ZEROS (type))
> +     @0))
> + )
> + /* (A - B) >=/> 0 ? (A - B) : (B - A)    same as abs (A - B) */
> + (for cmp (ge gt)
> +  (simplify
> +   (cnd (cmp (minus@0 @1 @2) zerop) @0 (minus @2 @1))
> +    (if (!HONOR_SIGNED_ZEROS (type)
> +        && !TYPE_UNSIGNED (type))
> +     (abs @0))))
> + /* (A - B) <=/< 0 ? (A - B) : (B - A)    same as -abs (A - B) */
> + (for cmp (le lt)
> +  (simplify
> +   (cnd (cmp (minus@0 @1 @2) zerop) @0 (minus @2 @1))
> +    (if (!HONOR_SIGNED_ZEROS (type)
> +        && !TYPE_UNSIGNED (type))
> +     (if (ANY_INTEGRAL_TYPE_P (type)
> +         && !TYPE_OVERFLOW_WRAPS (type))
> +      (with {
> +       tree utype = unsigned_type_for (type);
> +       }
> +       (convert (negate (absu:utype @0))))
> +       (negate (abs @0)))))
> + )
>  )
>
>  /* -(type)!A -> (type)A - 1.  */
> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-25.c b/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-25.c
> new file mode 100644
> index 00000000000..0f0e3170f8d
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-25.c
> @@ -0,0 +1,45 @@
> +/* { dg-options "-O2 -fno-signed-zeros -fdump-tree-phiopt" } */
> +int minus1(int a, int b)
> +{
> +  int c = a - b;
> +  if (c == 0) c = b - a;
> +  return c;
> +}
> +int minus2(int a, int b)
> +{
> +  int c = a - b;
> +  if (c != 0) c = b - a;
> +  return c;
> +}
> +int minus3(int a, int b)
> +{
> +  int c = a - b;
> +  if (c == 0) c = 0;
> +  else c = b - a;
> +  return c;
> +}
> +int minus4(int a, int b)
> +{
> +  int c;
> +  if (a == b) c = 0;
> +  else
> +    c = b - a;
> +  return c;
> +}
> +int abs0(int a, int b)
> +{
> +  int c = a - b;
> +  if (c <= 0) c = b - a;
> +  return c;
> +}
> +int negabs(int a, int b)
> +{
> +  int c = a - b;
> +  if (c >= 0) c = b - a;
> +  return c;
> +}
> +
> +/* The above should be optimized at phiopt1 except for negabs which has to wait
> +  until phiopt2 as -abs is not acceptable in early phiopt.  */
> +/* { dg-final { scan-tree-dump-times "if" 1  "phiopt1"  } } */
> +/* { dg-final { scan-tree-dump-not "if" "phiopt2" } } */
> --
> 2.27.0
>