Re: [PATCH 5/5] Port most of the A CMP 0 ? A : -A to match

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

On Sun, Jul 4, 2021 at 8:42 PM apinski--- via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> From: Andrew Pinski <apinski@marvell.com>
>
> To improve phiopt and be able to remove abs_replacement, this ports
> most of "A CMP 0 ? A : -A" from fold_cond_expr_with_comparison to
> match.pd.  There is a few extra changes that are needed to remove
> the "A CMP 0 ? A : -A" part from fold_cond_expr_with_comparison:
>    * Need to handle (A - B) case
>    * Need to handle UN* comparisons.
>
> I will handle those in a different patch.
>
> Note phi-opt-15.c test needed to be updated as we get ABSU now
> instead of not getting ABS.  When ABSU was added phiopt was not
> updated even to use ABSU instead of not creating ABS.
>
> OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.

OK

> gcc/ChangeLog:
>
>         PR tree-optimization/101039
>         * match.pd (A CMP 0 ? A : -A): New patterns.
>         * tree-ssa-phiopt.c (abs_replacement): Delete function.
>         (tree_ssa_phiopt_worker): Don't call abs_replacement.
>         Update comment about abs_replacement.
>
> gcc/testsuite/ChangeLog:
>
>         PR tree-optimization/101039
>         * gcc.dg/tree-ssa/phi-opt-15.c: Update test to expect
>         ABSU and still not expect ABS_EXPR.
>         * gcc.dg/tree-ssa/phi-opt-23.c: New test.
>         * gcc.dg/tree-ssa/phi-opt-24.c: New test.
> ---
>  gcc/match.pd                               |  60 +++++++++
>  gcc/testsuite/gcc.dg/tree-ssa/phi-opt-15.c |   4 +-
>  gcc/testsuite/gcc.dg/tree-ssa/phi-opt-23.c |  44 +++++++
>  gcc/testsuite/gcc.dg/tree-ssa/phi-opt-24.c |  44 +++++++
>  gcc/tree-ssa-phiopt.c                      | 134 +--------------------
>  5 files changed, 152 insertions(+), 134 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/phi-opt-23.c
>  create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/phi-opt-24.c
>
> diff --git a/gcc/match.pd b/gcc/match.pd
> index 4e10d54383c..72860fbd448 100644
> --- a/gcc/match.pd
> +++ b/gcc/match.pd
> @@ -3976,6 +3976,66 @@ 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.
> +
> +   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.  */
> +
> +(for cnd (cond vec_cond)
> + /* A == 0 ? A : -A    same as -A */
> + (for cmp (eq uneq)
> +  (simplify
> +   (cnd (cmp @0 zerop) @0 (negate@1 @0))
> +    (if (!HONOR_SIGNED_ZEROS (type))
> +     @1))
> +  (simplify
> +   (cnd (cmp @0 zerop) integer_zerop (negate@1 @0))
> +    (if (!HONOR_SIGNED_ZEROS (type))
> +     @1))
> + )
> + /* A != 0 ? A : -A    same as A */
> + (for cmp (ne ltgt)
> +  (simplify
> +   (cnd (cmp @0 zerop) @0 (negate @0))
> +    (if (!HONOR_SIGNED_ZEROS (type))
> +     @0))
> +  (simplify
> +   (cnd (cmp @0 zerop) @0 integer_zerop)
> +    (if (!HONOR_SIGNED_ZEROS (type))
> +     @0))
> + )
> + /* A >=/> 0 ? A : -A    same as abs (A) */
> + (for cmp (ge gt)
> +  (simplify
> +   (cnd (cmp @0 zerop) @0 (negate @0))
> +    (if (!HONOR_SIGNED_ZEROS (type)
> +        && !TYPE_UNSIGNED (type))
> +     (abs @0))))
> + /* A <=/< 0 ? A : -A    same as -abs (A) */
> + (for cmp (le lt)
> +  (simplify
> +   (cnd (cmp @0 zerop) @0 (negate @0))
> +    (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.  */
>  (simplify
>   (negate (convert?:s (logical_inverted_value:s @0)))
> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-15.c b/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-15.c
> index ac3018ef533..6aec68961cf 100644
> --- a/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-15.c
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-15.c
> @@ -9,4 +9,6 @@ foo (int i)
>    return i;
>  }
>
> -/* { dg-final { scan-tree-dump-not "ABS" "optimized" } } */
> +/* We should not have ABS_EXPR but ABSU_EXPR instead. */
> +/* { dg-final { scan-tree-dump-not "ABS_EXPR" "optimized" } } */
> +/* { dg-final { scan-tree-dump "ABSU" "optimized" } } */
> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-23.c b/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-23.c
> new file mode 100644
> index 00000000000..ff658cd16a7
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-23.c
> @@ -0,0 +1,44 @@
> +/* { dg-options "-O2 -fdump-tree-phiopt" } */
> +
> +int f0(int A)
> +{
> +//     A == 0? A : -A    same as -A
> +  if (A == 0)  return A;
> +  return -A;
> +}
> +
> +int f1(int A)
> +{
> +//     A != 0? A : -A    same as A
> +  if (A != 0)  return A;
> +  return -A;
> +}
> +int f2(int A)
> +{
> +//     A >= 0? A : -A    same as abs (A)
> +  if (A >= 0)  return A;
> +  return -A;
> +}
> +int f3(int A)
> +{
> +//     A > 0?  A : -A    same as abs (A)
> +  if (A > 0)  return A;
> +  return -A;
> +}
> +int f4(int A)
> +{
> +//     A <= 0? A : -A    same as -abs (A)
> +  if (A <= 0)  return A;
> +  return -A;
> +}
> +int f5(int A)
> +{
> +//     A < 0?  A : -A    same as -abs (A)
> +  if (A < 0)  return A;
> +  return -A;
> +}
> +
> +/* These should be optimized in phiopt1 but is confused by predicts. */
> +/* { dg-final { scan-tree-dump-not "if" "phiopt1" { xfail *-*-* } } } */
> +/* { dg-final { scan-tree-dump-not "if" "phiopt2" } } */
> +
> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-24.c b/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-24.c
> new file mode 100644
> index 00000000000..eb89decb4bf
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-24.c
> @@ -0,0 +1,44 @@
> +/* { dg-options "-O2 -fno-signed-zeros -fdump-tree-phiopt" } */
> +
> +float f0(float A)
> +{
> +//     A == 0? A : -A    same as -A
> +  if (A == 0)  return A;
> +  return -A;
> +}
> +
> +float f1(float A)
> +{
> +//     A != 0? A : -A    same as A
> +  if (A != 0)  return A;
> +  return -A;
> +}
> +float f2(float A)
> +{
> +//     A >= 0? A : -A    same as abs (A)
> +  if (A >= 0)  return A;
> +  return -A;
> +}
> +float f3(float A)
> +{
> +//     A > 0?  A : -A    same as abs (A)
> +  if (A > 0)  return A;
> +  return -A;
> +}
> +float f4(float A)
> +{
> +//     A <= 0? A : -A    same as -abs (A)
> +  if (A <= 0)  return A;
> +  return -A;
> +}
> +float f5(float A)
> +{
> +//     A < 0?  A : -A    same as -abs (A)
> +  if (A < 0)  return A;
> +  return -A;
> +}
> +
> +/* These should be optimized in phiopt1 but is confused by predicts. */
> +/* { dg-final { scan-tree-dump-not "if" "phiopt1" { xfail *-*-* } } } */
> +/* { dg-final { scan-tree-dump-not "if" "phiopt2" } } */
> +
> diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c
> index fec8c02c062..a1664c1f914 100644
> --- a/gcc/tree-ssa-phiopt.c
> +++ b/gcc/tree-ssa-phiopt.c
> @@ -64,8 +64,6 @@ static int value_replacement (basic_block, basic_block,
>                               edge, edge, gphi *, tree, tree);
>  static bool minmax_replacement (basic_block, basic_block,
>                                 edge, edge, gphi *, tree, tree);
> -static bool abs_replacement (basic_block, basic_block,
> -                            edge, edge, gphi *, tree, tree);
>  static bool spaceship_replacement (basic_block, basic_block,
>                                    edge, edge, gphi *, tree, tree);
>  static bool cond_removal_in_popcount_clz_ctz_pattern (basic_block, basic_block,
> @@ -352,8 +350,6 @@ tree_ssa_phiopt_worker (bool do_store_elim, bool do_hoist_loads, bool early_p)
>                                                arg0, arg1,
>                                                early_p))
>             cfgchanged = true;
> -         else if (abs_replacement (bb, bb1, e1, e2, phi, arg0, arg1))
> -           cfgchanged = true;
>           else if (!early_p
>                    && cond_removal_in_popcount_clz_ctz_pattern (bb, bb1, e1,
>                                                                 e2, phi, arg0,
> @@ -2614,134 +2610,6 @@ cond_removal_in_popcount_clz_ctz_pattern (basic_block cond_bb,
>    return true;
>  }
>
> -/*  The function absolute_replacement does the main work of doing the absolute
> -    replacement.  Return true if the replacement is done.  Otherwise return
> -    false.
> -    bb is the basic block where the replacement is going to be done on.  arg0
> -    is argument 0 from the phi.  Likewise for arg1.  */
> -
> -static bool
> -abs_replacement (basic_block cond_bb, basic_block middle_bb,
> -                edge e0 ATTRIBUTE_UNUSED, edge e1,
> -                gphi *phi, tree arg0, tree arg1)
> -{
> -  tree result;
> -  gassign *new_stmt;
> -  gimple *cond;
> -  gimple_stmt_iterator gsi;
> -  edge true_edge, false_edge;
> -  gimple *assign;
> -  edge e;
> -  tree rhs, lhs;
> -  bool negate;
> -  enum tree_code cond_code;
> -
> -  /* If the type says honor signed zeros we cannot do this
> -     optimization.  */
> -  if (HONOR_SIGNED_ZEROS (arg1))
> -    return false;
> -
> -  /* OTHER_BLOCK must have only one executable statement which must have the
> -     form arg0 = -arg1 or arg1 = -arg0.  */
> -
> -  assign = last_and_only_stmt (middle_bb);
> -  /* If we did not find the proper negation assignment, then we cannot
> -     optimize.  */
> -  if (assign == NULL)
> -    return false;
> -
> -  /* If we got here, then we have found the only executable statement
> -     in OTHER_BLOCK.  If it is anything other than arg = -arg1 or
> -     arg1 = -arg0, then we cannot optimize.  */
> -  if (gimple_code (assign) != GIMPLE_ASSIGN)
> -    return false;
> -
> -  lhs = gimple_assign_lhs (assign);
> -
> -  if (gimple_assign_rhs_code (assign) != NEGATE_EXPR)
> -    return false;
> -
> -  rhs = gimple_assign_rhs1 (assign);
> -
> -  /* The assignment has to be arg0 = -arg1 or arg1 = -arg0.  */
> -  if (!(lhs == arg0 && rhs == arg1)
> -      && !(lhs == arg1 && rhs == arg0))
> -    return false;
> -
> -  cond = last_stmt (cond_bb);
> -  result = PHI_RESULT (phi);
> -
> -  /* Only relationals comparing arg[01] against zero are interesting.  */
> -  cond_code = gimple_cond_code (cond);
> -  if (cond_code != GT_EXPR && cond_code != GE_EXPR
> -      && cond_code != LT_EXPR && cond_code != LE_EXPR)
> -    return false;
> -
> -  /* Make sure the conditional is arg[01] OP y.  */
> -  if (gimple_cond_lhs (cond) != rhs)
> -    return false;
> -
> -  if (FLOAT_TYPE_P (TREE_TYPE (gimple_cond_rhs (cond)))
> -              ? real_zerop (gimple_cond_rhs (cond))
> -              : integer_zerop (gimple_cond_rhs (cond)))
> -    ;
> -  else
> -    return false;
> -
> -  /* We need to know which is the true edge and which is the false
> -     edge so that we know if have abs or negative abs.  */
> -  extract_true_false_edges_from_block (cond_bb, &true_edge, &false_edge);
> -
> -  /* For GT_EXPR/GE_EXPR, if the true edge goes to OTHER_BLOCK, then we
> -     will need to negate the result.  Similarly for LT_EXPR/LE_EXPR if
> -     the false edge goes to OTHER_BLOCK.  */
> -  if (cond_code == GT_EXPR || cond_code == GE_EXPR)
> -    e = true_edge;
> -  else
> -    e = false_edge;
> -
> -  if (e->dest == middle_bb)
> -    negate = true;
> -  else
> -    negate = false;
> -
> -  /* If the code negates only iff positive then make sure to not
> -     introduce undefined behavior when negating or computing the absolute.
> -     ???  We could use range info if present to check for arg1 == INT_MIN.  */
> -  if (negate
> -      && (ANY_INTEGRAL_TYPE_P (TREE_TYPE (arg1))
> -         && ! TYPE_OVERFLOW_WRAPS (TREE_TYPE (arg1))))
> -    return false;
> -
> -  result = duplicate_ssa_name (result, NULL);
> -
> -  if (negate)
> -    lhs = make_ssa_name (TREE_TYPE (result));
> -  else
> -    lhs = result;
> -
> -  /* Build the modify expression with abs expression.  */
> -  new_stmt = gimple_build_assign (lhs, ABS_EXPR, rhs);
> -
> -  gsi = gsi_last_bb (cond_bb);
> -  gsi_insert_before (&gsi, new_stmt, GSI_NEW_STMT);
> -
> -  if (negate)
> -    {
> -      /* Get the right GSI.  We want to insert after the recently
> -        added ABS_EXPR statement (which we know is the first statement
> -        in the block.  */
> -      new_stmt = gimple_build_assign (result, NEGATE_EXPR, lhs);
> -
> -      gsi_insert_after (&gsi, new_stmt, GSI_NEW_STMT);
> -    }
> -
> -  replace_phi_edge_with_variable (cond_bb, e1, phi, result);
> -
> -  /* Note that we optimized this PHI.  */
> -  return true;
> -}
> -
>  /* Auxiliary functions to determine the set of memory accesses which
>     can't trap because they are preceded by accesses to the same memory
>     portion.  We do that for MEM_REFs, so we only need to track
> @@ -3678,7 +3546,7 @@ gate_hoist_loads (void)
>     ABS Replacement
>     ---------------
>
> -   This transformation, implemented in abs_replacement, replaces
> +   This transformation, implemented in match_simplify_replacement, replaces
>
>       bb0:
>         if (a >= 0) goto bb2; else goto bb1;
> --
> 2.27.0
>