[PATCH 0/7] PHI-OPT move abs_replacement to match.pd

[PATCH 0/7] PHI-OPT move abs_replacement to match.pd

[apinski]

From: Andrew Pinski <apinski@marvell.com>

To able to move PHI-OPT's abs_replacement to match.pd, a bunch
of support needed to be added to PHI-OPT.
This is a set of 7 patches which allows us to remove abs_replacement
and even does one set further and does a few extra transformations
that abs_replacement did not do (just because of the moving from
fold to match).

Andrew Pinski (7):
  Expand the comparison argument of fold_cond_expr_with_comparison
  Reset the range info on the moved instruction in PHIOPT
  Duplicate the range information of the phi onto the new ssa_name
  Allow match-and-simplified phiopt to run in early phiopt
  Try inverted comparison for match_simplify in phiopt
  Lower for loops before lowering cond in genmatch
  Port most of the A CMP 0 ? A : -A to match

 gcc/fold-const.c                           |  39 +--
 gcc/genmatch.c                             |  28 +-
 gcc/match.pd                               |  60 +++++
 gcc/testsuite/gcc.dg/tree-ssa/phi-opt-15.c |   4 +-
 gcc/tree-ssa-phiopt.c                      | 286 +++++++++------------
 5 files changed, 217 insertions(+), 200 deletions(-)

-- 
2.27.0

[PATCH 1/7] Expand the comparison argument of fold_cond_expr_with_comparison

[apinski]

From: Andrew Pinski <apinski@marvell.com>

To make things slightly easiler to convert fold_cond_expr_with_comparison
over to match.pd, expanding the arg0 argument into 3 different arguments
is done. Also this was simple because we don't use arg0 after grabbing
the code and the two operands.
Also since we do this, we don't need to fold the comparison to
get the inverse but just use invert_tree_comparison directly.

OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.

gcc/ChangeLog:

	* fold-const.c (fold_cond_expr_with_comparison):
	Exand arg0 into comp_code, arg00, and arg01.
	(fold_ternary_loc): Use invert_tree_comparison
	instead of fold_invert_truthvalue for the case
	where we have A CMP B ? C : A.
---
 gcc/fold-const.c | 39 ++++++++++++++++++++++-----------------
 1 file changed, 22 insertions(+), 17 deletions(-)

diff --git a/gcc/fold-const.c b/gcc/fold-const.c
index 0b33ee99a81..5e4bdeace1e 100644
--- a/gcc/fold-const.c
+++ b/gcc/fold-const.c
@@ -126,7 +126,8 @@ static tree range_binop (enum tree_code, tree, tree, int, tree, int);
 static tree range_predecessor (tree);
 static tree range_successor (tree);
 static tree fold_range_test (location_t, enum tree_code, tree, tree, tree);
-static tree fold_cond_expr_with_comparison (location_t, tree, tree, tree, tree);
+static tree fold_cond_expr_with_comparison (location_t, tree, enum tree_code,
+					    tree, tree, tree, tree);
 static tree unextend (tree, int, int, tree);
 static tree extract_muldiv (tree, tree, enum tree_code, tree, bool *);
 static tree extract_muldiv_1 (tree, tree, enum tree_code, tree, bool *);
@@ -5735,20 +5736,19 @@ merge_ranges (int *pin_p, tree *plow, tree *phigh, int in0_p, tree low0,
 \f
 
 /* Subroutine of fold, looking inside expressions of the form
-   A op B ? A : C, where ARG0, ARG1 and ARG2 are the three operands
-   of the COND_EXPR.  This function is being used also to optimize
-   A op B ? C : A, by reversing the comparison first.
+   A op B ? A : C, where (ARG00, COMP_CODE, ARG01), ARG1 and ARG2
+   are the three operands of the COND_EXPR.  This function is
+   being used also to optimize A op B ? C : A, by reversing the
+   comparison first.
 
    Return a folded expression whose code is not a COND_EXPR
    anymore, or NULL_TREE if no folding opportunity is found.  */
 
 static tree
 fold_cond_expr_with_comparison (location_t loc, tree type,
-				tree arg0, tree arg1, tree arg2)
+				enum tree_code comp_code,
+				tree arg00, tree arg01, tree arg1, tree arg2)
 {
-  enum tree_code comp_code = TREE_CODE (arg0);
-  tree arg00 = TREE_OPERAND (arg0, 0);
-  tree arg01 = TREE_OPERAND (arg0, 1);
   tree arg1_type = TREE_TYPE (arg1);
   tree tem;
 
@@ -12822,7 +12822,10 @@ fold_ternary_loc (location_t loc, enum tree_code code, tree type,
 	  && operand_equal_for_comparison_p (TREE_OPERAND (arg0, 0), op1)
 	  && !HONOR_SIGNED_ZEROS (element_mode (op1)))
 	{
-	  tem = fold_cond_expr_with_comparison (loc, type, arg0, op1, op2);
+	  tem = fold_cond_expr_with_comparison (loc, type, TREE_CODE (arg0),
+						TREE_OPERAND (arg0, 0),
+						TREE_OPERAND (arg0, 1),
+						op1, op2);
 	  if (tem)
 	    return tem;
 	}
@@ -12831,14 +12834,16 @@ fold_ternary_loc (location_t loc, enum tree_code code, tree type,
 	  && operand_equal_for_comparison_p (TREE_OPERAND (arg0, 0), op2)
 	  && !HONOR_SIGNED_ZEROS (element_mode (op2)))
 	{
-	  location_t loc0 = expr_location_or (arg0, loc);
-	  tem = fold_invert_truthvalue (loc0, arg0);
-	  if (tem && COMPARISON_CLASS_P (tem))
-	    {
-	      tem = fold_cond_expr_with_comparison (loc, type, tem, op2, op1);
-	      if (tem)
-		return tem;
-	    }
+	  enum tree_code comp_code = TREE_CODE (arg0);
+	  tree arg00 = TREE_OPERAND (arg0, 0);
+	  tree arg01 = TREE_OPERAND (arg0, 1);
+	  comp_code = invert_tree_comparison (comp_code, HONOR_NANS (arg00));
+	  tem = fold_cond_expr_with_comparison (loc, type, comp_code,
+						arg00,
+						arg01,
+						op2, op1);
+	  if (tem)
+	    return tem;
 	}
 
       /* If the second operand is simpler than the third, swap them
-- 
2.27.0

Re: [PATCH 1/7] Expand the comparison argument of fold_cond_expr_with_comparison

[Jeff Law]

On 6/23/2021 4:19 PM, apinski--- via Gcc-patches wrote:
> From: Andrew Pinski <apinski@marvell.com>
>
> To make things slightly easiler to convert fold_cond_expr_with_comparison
> over to match.pd, expanding the arg0 argument into 3 different arguments
> is done. Also this was simple because we don't use arg0 after grabbing
> the code and the two operands.
> Also since we do this, we don't need to fold the comparison to
> get the inverse but just use invert_tree_comparison directly.
>
> OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.
>
> gcc/ChangeLog:
>
> 	* fold-const.c (fold_cond_expr_with_comparison):
> 	Exand arg0 into comp_code, arg00, and arg01.
> 	(fold_ternary_loc): Use invert_tree_comparison
> 	instead of fold_invert_truthvalue for the case
> 	where we have A CMP B ? C : A.
OK
jeff

[PATCH 2/7] Reset the range info on the moved instruction in PHIOPT

[apinski]

From: Andrew Pinski <apinski@marvell.com>

I had missed this when wrote the patch which allowed the
gimple to be moved from inside the conditional as it.  It
was also missed in the review.  Anyways the range information
needs to be reset for the moved gimple as it was under a
conditional and the flow has changed to be unconditional.
I have not seen any testcase in the wild that produces wrong code
yet which is why there is no testcase but this is similar to what
the other code in phiopt does so after moving those to match, there
might be some.

OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.

gcc/ChangeLog:

	* tree-ssa-phiopt.c (match_simplify_replacement): Reset
	flow senatitive info on the moved ssa set.
---
 gcc/tree-ssa-phiopt.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c
index 02e26f974a5..24cbce9955a 100644
--- a/gcc/tree-ssa-phiopt.c
+++ b/gcc/tree-ssa-phiopt.c
@@ -836,7 +836,7 @@ match_simplify_replacement (basic_block cond_bb, basic_block middle_bb,
       if (!is_gimple_assign (stmt_to_move))
 	return false;
 
-      tree lhs = gimple_assign_lhs  (stmt_to_move);
+      tree lhs = gimple_assign_lhs (stmt_to_move);
       gimple *use_stmt;
       use_operand_p use_p;
 
@@ -892,6 +892,7 @@ match_simplify_replacement (basic_block cond_bb, basic_block middle_bb,
 	}
       gimple_stmt_iterator gsi1 = gsi_for_stmt (stmt_to_move);
       gsi_move_before (&gsi1, &gsi);
+      reset_flow_sensitive_info (gimple_assign_lhs (stmt_to_move));
     }
   if (seq)
     gsi_insert_seq_before (&gsi, seq, GSI_SAME_STMT);
-- 
2.27.0

Re: [PATCH 2/7] Reset the range info on the moved instruction in PHIOPT

[Jeff Law]

On 6/23/2021 4:19 PM, apinski--- via Gcc-patches wrote:
> From: Andrew Pinski <apinski@marvell.com>
>
> I had missed this when wrote the patch which allowed the
> gimple to be moved from inside the conditional as it.  It
> was also missed in the review.  Anyways the range information
> needs to be reset for the moved gimple as it was under a
> conditional and the flow has changed to be unconditional.
> I have not seen any testcase in the wild that produces wrong code
> yet which is why there is no testcase but this is similar to what
> the other code in phiopt does so after moving those to match, there
> might be some.
>
> OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.
>
> gcc/ChangeLog:
>
> 	* tree-ssa-phiopt.c (match_simplify_replacement): Reset
> 	flow senatitive info on the moved ssa set.
OK
jeff

[PATCH 3/7] Duplicate the range information of the phi onto the new ssa_name

[apinski]

From: Andrew Pinski <apinski@marvell.com>

Since match_simplify_replacement uses gimple_simplify, there is a new
ssa name created sometimes and then we go and replace the phi edge with
this new ssa name, the range information on the phi is lost.
Placing this in replace_phi_edge_with_variable is the best option instead
of doing it in each time replace_phi_edge_with_variable is called which is
what is done today.

OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.

gcc/ChangeLog:

	* tree-ssa-phiopt.c (replace_phi_edge_with_variable): Duplicate range
	info if we're the only things setting the target PHI.
	(value_replacement): Don't duplicate range here.
	(minmax_replacement): Likewise.
---
 gcc/tree-ssa-phiopt.c | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c
index 24cbce9955a..147397ad82c 100644
--- a/gcc/tree-ssa-phiopt.c
+++ b/gcc/tree-ssa-phiopt.c
@@ -391,6 +391,24 @@ replace_phi_edge_with_variable (basic_block cond_block,
   basic_block bb = gimple_bb (phi);
   basic_block block_to_remove;
   gimple_stmt_iterator gsi;
+  tree phi_result = PHI_RESULT (phi);
+
+  /* Duplicate range info as needed.  */
+  if (TREE_CODE (new_tree) == SSA_NAME
+      && EDGE_COUNT (gimple_bb (phi)->preds) == 2
+      && INTEGRAL_TYPE_P (TREE_TYPE (phi_result)))
+    {
+      if (!SSA_NAME_RANGE_INFO (new_tree)
+	  && SSA_NAME_RANGE_INFO (phi_result))
+	duplicate_ssa_name_range_info (new_tree,
+				       SSA_NAME_RANGE_TYPE (phi_result),
+				       SSA_NAME_RANGE_INFO (phi_result));
+      if (SSA_NAME_RANGE_INFO (new_tree)
+	  && !SSA_NAME_RANGE_INFO (phi_result))
+	duplicate_ssa_name_range_info (phi_result,
+				       SSA_NAME_RANGE_TYPE (new_tree),
+				       SSA_NAME_RANGE_INFO (new_tree));
+    }
 
   /* Change the PHI argument to new.  */
   SET_USE (PHI_ARG_DEF_PTR (phi, e->dest_idx), new_tree);
@@ -1385,16 +1403,6 @@ value_replacement (basic_block cond_bb, basic_block middle_bb,
 	   <bb 4>:
 	   # u_3 = PHI <u_6(3), 4294967295(2)>  */
       reset_flow_sensitive_info (lhs);
-      if (INTEGRAL_TYPE_P (TREE_TYPE (lhs)))
-	{
-	  /* If available, we can use VR of phi result at least.  */
-	  tree phires = gimple_phi_result (phi);
-	  struct range_info_def *phires_range_info
-	    = SSA_NAME_RANGE_INFO (phires);
-	  if (phires_range_info)
-	    duplicate_ssa_name_range_info (lhs, SSA_NAME_RANGE_TYPE (phires),
-					   phires_range_info);
-	}
       gimple_stmt_iterator gsi_from;
       for (int i = prep_cnt - 1; i >= 0; --i)
 	{
@@ -1794,13 +1802,6 @@ minmax_replacement (basic_block cond_bb, basic_block middle_bb,
   gimple_seq stmts = NULL;
   tree phi_result = PHI_RESULT (phi);
   result = gimple_build (&stmts, minmax, TREE_TYPE (phi_result), arg0, arg1);
-  /* Duplicate range info if we're the only things setting the target PHI.  */
-  if (!gimple_seq_empty_p (stmts)
-      && EDGE_COUNT (gimple_bb (phi)->preds) == 2
-      && !POINTER_TYPE_P (TREE_TYPE (phi_result))
-      && SSA_NAME_RANGE_INFO (phi_result))
-    duplicate_ssa_name_range_info (result, SSA_NAME_RANGE_TYPE (phi_result),
-				   SSA_NAME_RANGE_INFO (phi_result));
 
   gsi = gsi_last_bb (cond_bb);
   gsi_insert_seq_before (&gsi, stmts, GSI_NEW_STMT);
-- 
2.27.0

Re: [PATCH 3/7] Duplicate the range information of the phi onto the new ssa_name

[Jeff Law]

On 6/23/2021 4:19 PM, apinski--- via Gcc-patches wrote:
> From: Andrew Pinski <apinski@marvell.com>
>
> Since match_simplify_replacement uses gimple_simplify, there is a new
> ssa name created sometimes and then we go and replace the phi edge with
> this new ssa name, the range information on the phi is lost.
> Placing this in replace_phi_edge_with_variable is the best option instead
> of doing it in each time replace_phi_edge_with_variable is called which is
> what is done today.
>
> OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.
>
> gcc/ChangeLog:
>
> 	* tree-ssa-phiopt.c (replace_phi_edge_with_variable): Duplicate range
> 	info if we're the only things setting the target PHI.
> 	(value_replacement): Don't duplicate range here.
> 	(minmax_replacement): Likewise.
> ---
>   gcc/tree-ssa-phiopt.c | 35 ++++++++++++++++++-----------------
>   1 file changed, 18 insertions(+), 17 deletions(-)
>
> diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c
> index 24cbce9955a..147397ad82c 100644
> --- a/gcc/tree-ssa-phiopt.c
> +++ b/gcc/tree-ssa-phiopt.c
> @@ -391,6 +391,24 @@ replace_phi_edge_with_variable (basic_block cond_block,
>     basic_block bb = gimple_bb (phi);
>     basic_block block_to_remove;
>     gimple_stmt_iterator gsi;
> +  tree phi_result = PHI_RESULT (phi);
> +
> +  /* Duplicate range info as needed.  */
> +  if (TREE_CODE (new_tree) == SSA_NAME
> +      && EDGE_COUNT (gimple_bb (phi)->preds) == 2
> +      && INTEGRAL_TYPE_P (TREE_TYPE (phi_result)))
> +    {
> +      if (!SSA_NAME_RANGE_INFO (new_tree)
> +	  && SSA_NAME_RANGE_INFO (phi_result))
> +	duplicate_ssa_name_range_info (new_tree,
> +				       SSA_NAME_RANGE_TYPE (phi_result),
> +				       SSA_NAME_RANGE_INFO (phi_result));
> +      if (SSA_NAME_RANGE_INFO (new_tree)
> +	  && !SSA_NAME_RANGE_INFO (phi_result))
> +	duplicate_ssa_name_range_info (phi_result,
> +				       SSA_NAME_RANGE_TYPE (new_tree),
> +				       SSA_NAME_RANGE_INFO (new_tree));
> +    }
I'm not sure you need the EDGE_COUNT test here.

I worry that copying the range info from the argument to the PHI result 
isn't right.  It was OK for the ABS replacement, but I'm not sure that's 
true in general.

Jeff


>   
>     /* Change the PHI argument to new.  */
>     SET_USE (PHI_ARG_DEF_PTR (phi, e->dest_idx), new_tree);
> @@ -1385,16 +1403,6 @@ value_replacement (basic_block cond_bb, basic_block middle_bb,
>   	   <bb 4>:
>   	   # u_3 = PHI <u_6(3), 4294967295(2)>  */
>         reset_flow_sensitive_info (lhs);
> -      if (INTEGRAL_TYPE_P (TREE_TYPE (lhs)))
> -	{
> -	  /* If available, we can use VR of phi result at least.  */
> -	  tree phires = gimple_phi_result (phi);
> -	  struct range_info_def *phires_range_info
> -	    = SSA_NAME_RANGE_INFO (phires);
> -	  if (phires_range_info)
> -	    duplicate_ssa_name_range_info (lhs, SSA_NAME_RANGE_TYPE (phires),
> -					   phires_range_info);
> -	}
>         gimple_stmt_iterator gsi_from;
>         for (int i = prep_cnt - 1; i >= 0; --i)
>   	{
> @@ -1794,13 +1802,6 @@ minmax_replacement (basic_block cond_bb, basic_block middle_bb,
>     gimple_seq stmts = NULL;
>     tree phi_result = PHI_RESULT (phi);
>     result = gimple_build (&stmts, minmax, TREE_TYPE (phi_result), arg0, arg1);
> -  /* Duplicate range info if we're the only things setting the target PHI.  */
> -  if (!gimple_seq_empty_p (stmts)
> -      && EDGE_COUNT (gimple_bb (phi)->preds) == 2
> -      && !POINTER_TYPE_P (TREE_TYPE (phi_result))
> -      && SSA_NAME_RANGE_INFO (phi_result))
> -    duplicate_ssa_name_range_info (result, SSA_NAME_RANGE_TYPE (phi_result),
> -				   SSA_NAME_RANGE_INFO (phi_result));
>   
>     gsi = gsi_last_bb (cond_bb);
>     gsi_insert_seq_before (&gsi, stmts, GSI_NEW_STMT);

Re: [PATCH 3/7] Duplicate the range information of the phi onto the new ssa_name

[Andrew Pinski]

On Thu, Jun 24, 2021 at 8:17 AM Jeff Law via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
>
>
> On 6/23/2021 4:19 PM, apinski--- via Gcc-patches wrote:
> > From: Andrew Pinski <apinski@marvell.com>
> >
> > Since match_simplify_replacement uses gimple_simplify, there is a new
> > ssa name created sometimes and then we go and replace the phi edge with
> > this new ssa name, the range information on the phi is lost.
> > Placing this in replace_phi_edge_with_variable is the best option instead
> > of doing it in each time replace_phi_edge_with_variable is called which is
> > what is done today.
> >
> > OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.
> >
> > gcc/ChangeLog:
> >
> >       * tree-ssa-phiopt.c (replace_phi_edge_with_variable): Duplicate range
> >       info if we're the only things setting the target PHI.
> >       (value_replacement): Don't duplicate range here.
> >       (minmax_replacement): Likewise.
> > ---
> >   gcc/tree-ssa-phiopt.c | 35 ++++++++++++++++++-----------------
> >   1 file changed, 18 insertions(+), 17 deletions(-)
> >
> > diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c
> > index 24cbce9955a..147397ad82c 100644
> > --- a/gcc/tree-ssa-phiopt.c
> > +++ b/gcc/tree-ssa-phiopt.c
> > @@ -391,6 +391,24 @@ replace_phi_edge_with_variable (basic_block cond_block,
> >     basic_block bb = gimple_bb (phi);
> >     basic_block block_to_remove;
> >     gimple_stmt_iterator gsi;
> > +  tree phi_result = PHI_RESULT (phi);
> > +
> > +  /* Duplicate range info as needed.  */
> > +  if (TREE_CODE (new_tree) == SSA_NAME
> > +      && EDGE_COUNT (gimple_bb (phi)->preds) == 2
> > +      && INTEGRAL_TYPE_P (TREE_TYPE (phi_result)))
> > +    {
> > +      if (!SSA_NAME_RANGE_INFO (new_tree)
> > +       && SSA_NAME_RANGE_INFO (phi_result))
> > +     duplicate_ssa_name_range_info (new_tree,
> > +                                    SSA_NAME_RANGE_TYPE (phi_result),
> > +                                    SSA_NAME_RANGE_INFO (phi_result));
> > +      if (SSA_NAME_RANGE_INFO (new_tree)
> > +       && !SSA_NAME_RANGE_INFO (phi_result))
> > +     duplicate_ssa_name_range_info (phi_result,
> > +                                    SSA_NAME_RANGE_TYPE (new_tree),
> > +                                    SSA_NAME_RANGE_INFO (new_tree));
> > +    }
> I'm not sure you need the EDGE_COUNT test here.

I think we need it. See below for the reason why.

>
> I worry that copying the range info from the argument to the PHI result
> isn't right.  It was OK for the ABS replacement, but I'm not sure that's
> true in general.

Right, I had originally placed the code that was done in
minmax_replacement in match_simplify_replacement.
But Richi wanted me to move the code to replace_phi_edge_with_variable
and have it be able to go both directions for the range.
The reason why the check for the number of incoming edges is needed is
because if we start with:

Range[xyz]
c_1 = PHI <a_2(3), b_3(4), c_4(5)>

And we are replacing the variable for edges 4 and 5 only, then we
would get the wrong range on the new SSA name as it would include the
range of a_2 which is not correct.
Also we would not lose the range information in that case either.
This is more about losing the range information.

In the case where this shows up is we have:
bb1:
if (a_1 >= 255) goto bb2; else goto bb3;

bb2:
 a_2 = 255;
goto bb3;

range<-INF,255>
a_3 = PHI<a_1(1), a_2(2)>

Which gets translated into
bb1:
_4 = min<a_1, 255>
goto bb2

range<-INF,255>
a_3 = PHI<_4(1)>
bb3:

use(a_3)

And when cfg-cleanup merges the two basic blocks, _4 is propagated
into where a_3 was used and the range information is now gone.

So the idea is to propagate the range info from a_3 back on to _4 if
we know that they propagated later on.
The opposite way (range of _4 on to a_3) actually does not really
matter as we are going to throw away the PHI node anyways.

Thanks,
Andrew Pinski

>
> Jeff
>
>
> >
> >     /* Change the PHI argument to new.  */
> >     SET_USE (PHI_ARG_DEF_PTR (phi, e->dest_idx), new_tree);
> > @@ -1385,16 +1403,6 @@ value_replacement (basic_block cond_bb, basic_block middle_bb,
> >          <bb 4>:
> >          # u_3 = PHI <u_6(3), 4294967295(2)>  */
> >         reset_flow_sensitive_info (lhs);
> > -      if (INTEGRAL_TYPE_P (TREE_TYPE (lhs)))
> > -     {
> > -       /* If available, we can use VR of phi result at least.  */
> > -       tree phires = gimple_phi_result (phi);
> > -       struct range_info_def *phires_range_info
> > -         = SSA_NAME_RANGE_INFO (phires);
> > -       if (phires_range_info)
> > -         duplicate_ssa_name_range_info (lhs, SSA_NAME_RANGE_TYPE (phires),
> > -                                        phires_range_info);
> > -     }
> >         gimple_stmt_iterator gsi_from;
> >         for (int i = prep_cnt - 1; i >= 0; --i)
> >       {
> > @@ -1794,13 +1802,6 @@ minmax_replacement (basic_block cond_bb, basic_block middle_bb,
> >     gimple_seq stmts = NULL;
> >     tree phi_result = PHI_RESULT (phi);
> >     result = gimple_build (&stmts, minmax, TREE_TYPE (phi_result), arg0, arg1);
> > -  /* Duplicate range info if we're the only things setting the target PHI.  */
> > -  if (!gimple_seq_empty_p (stmts)
> > -      && EDGE_COUNT (gimple_bb (phi)->preds) == 2
> > -      && !POINTER_TYPE_P (TREE_TYPE (phi_result))
> > -      && SSA_NAME_RANGE_INFO (phi_result))
> > -    duplicate_ssa_name_range_info (result, SSA_NAME_RANGE_TYPE (phi_result),
> > -                                SSA_NAME_RANGE_INFO (phi_result));
> >
> >     gsi = gsi_last_bb (cond_bb);
> >     gsi_insert_seq_before (&gsi, stmts, GSI_NEW_STMT);
>

[PATCH 4/7] Allow match-and-simplified phiopt to run in early phiopt

[apinski]

From: Andrew Pinski <apinski@marvell.com>

To move a few things more to match-and-simplify from phiopt,
we need to allow match_simplify_replacement to run in early
phiopt. To do this we add a replacement for gimple_simplify
that is explictly for phiopt.

OK? Bootstrapped and tested on x86_64-linux-gnu with no
regressions.

gcc/ChangeLog:

	* tree-ssa-phiopt.c (match_simplify_replacement):
	Add early_p argument. Call gimple_simplify_phiopt
	instead of gimple_simplify.
	(tree_ssa_phiopt_worker): Update call to
	match_simplify_replacement and allow unconditionally.
	(phiopt_early_allow): New function.
	(gimple_simplify_phiopt): New function.
---
 gcc/tree-ssa-phiopt.c | 89 ++++++++++++++++++++++++++++++++++---------
 1 file changed, 70 insertions(+), 19 deletions(-)

diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c
index 147397ad82c..8b0e68c1e90 100644
--- a/gcc/tree-ssa-phiopt.c
+++ b/gcc/tree-ssa-phiopt.c
@@ -50,12 +50,13 @@ along with GCC; see the file COPYING3.  If not see
 #include "gimple-fold.h"
 #include "internal-fn.h"
 #include "gimple-range.h"
+#include "gimple-match.h"
 
 static unsigned int tree_ssa_phiopt_worker (bool, bool, bool);
 static bool two_value_replacement (basic_block, basic_block, edge, gphi *,
 				   tree, tree);
 static bool match_simplify_replacement (basic_block, basic_block,
-					edge, edge, gphi *, tree, tree);
+					edge, edge, gphi *, tree, tree, bool);
 static gphi *factor_out_conditional_conversion (edge, edge, gphi *, tree, tree,
 						gimple *);
 static int value_replacement (basic_block, basic_block,
@@ -345,9 +346,9 @@ tree_ssa_phiopt_worker (bool do_store_elim, bool do_hoist_loads, bool early_p)
 	  /* Do the replacement of conditional if it can be done.  */
 	  if (!early_p && two_value_replacement (bb, bb1, e2, phi, arg0, arg1))
 	    cfgchanged = true;
-	  else if (!early_p
-		   && match_simplify_replacement (bb, bb1, e1, e2, phi,
-						  arg0, arg1))
+	  else if (match_simplify_replacement (bb, bb1, e1, e2, phi,
+					       arg0, arg1,
+					       early_p))
 	    cfgchanged = true;
 	  else if (abs_replacement (bb, bb1, e1, e2, phi, arg0, arg1))
 	    cfgchanged = true;
@@ -803,6 +804,67 @@ two_value_replacement (basic_block cond_bb, basic_block middle_bb,
   return true;
 }
 
+/* Return TRUE if CODE should be allowed during early phiopt.
+   Currently this is to allow MIN/MAX and ABS/NEGATE.  */
+static bool
+phiopt_early_allow (enum tree_code code)
+{
+  switch (code)
+    {
+      case MIN_EXPR:
+      case MAX_EXPR:
+      case ABS_EXPR:
+      case ABSU_EXPR:
+      case NEGATE_EXPR:
+      case SSA_NAME:
+	return true;
+      default:
+	return false;
+    }
+}
+
+/* gimple_simplify_phiopt is like gimple_simplify but designed for PHIOPT.
+   Return NULL if nothing can be simplified or the resulting simplified value
+   with parts pushed if EARLY_P was true. Also rejects non allowed tree code
+   if EARLY_P is set.
+   Takes the comparison from COMP_STMT and two args, ARG0 and ARG1 and tries
+   to simplify CMP ? ARG0 : ARG1.  */
+static tree
+gimple_simplify_phiopt (bool early_p, tree type, gimple *comp_stmt,
+			tree arg0, tree arg1,
+			gimple_seq *seq)
+{
+  tree result;
+  enum tree_code comp_code = gimple_cond_code (comp_stmt);
+  location_t loc = gimple_location (comp_stmt);
+  tree cmp0 = gimple_cond_lhs (comp_stmt);
+  tree cmp1 = gimple_cond_rhs (comp_stmt);
+  /* To handle special cases like floating point comparison, it is easier and
+     less error-prone to build a tree and gimplify it on the fly though it is
+     less efficient.
+     Don't use fold_build2 here as that might create (bool)a instead of just
+     "a != 0".  */
+  tree cond = build2_loc (loc, comp_code, boolean_type_node,
+			  cmp0, cmp1);
+  gimple_match_op op (gimple_match_cond::UNCOND,
+		      COND_EXPR, type, cond, arg0, arg1);
+
+  if (op.resimplify (early_p ? NULL : seq, follow_all_ssa_edges))
+    {
+      /* Early we want only to allow some generated tree codes. */
+      if (!early_p
+	  || op.code.is_tree_code ()
+	  || phiopt_early_allow ((tree_code)op.code))
+	{
+	  result = maybe_push_res_to_seq (&op, seq);
+	  if (result)
+	    return result;
+	}
+    }
+
+  return NULL;
+}
+
 /*  The function match_simplify_replacement does the main work of doing the
     replacement using match and simplify.  Return true if the replacement is done.
     Otherwise return false.
@@ -812,10 +874,9 @@ two_value_replacement (basic_block cond_bb, basic_block middle_bb,
 static bool
 match_simplify_replacement (basic_block cond_bb, basic_block middle_bb,
 			    edge e0, edge e1, gphi *phi,
-			    tree arg0, tree arg1)
+			    tree arg0, tree arg1, bool early_p)
 {
   gimple *stmt;
-  tree cond;
   gimple_stmt_iterator gsi;
   edge true_edge, false_edge;
   gimple_seq seq = NULL;
@@ -876,15 +937,6 @@ match_simplify_replacement (basic_block cond_bb, basic_block middle_bb,
 
   stmt = last_stmt (cond_bb);
 
-  /* To handle special cases like floating point comparison, it is easier and
-     less error-prone to build a tree and gimplify it on the fly though it is
-     less efficient.
-     Don't use fold_build2 here as that might create (bool)a instead of just
-     "a != 0".  */
-  cond = build2_loc (gimple_location (stmt),
-		     gimple_cond_code (stmt), boolean_type_node,
-		     gimple_cond_lhs (stmt), gimple_cond_rhs (stmt));
-
   /* We need to know which is the true edge and which is the false
      edge so that we know when to invert the condition below.  */
   extract_true_false_edges_from_block (cond_bb, &true_edge, &false_edge);
@@ -892,10 +944,9 @@ match_simplify_replacement (basic_block cond_bb, basic_block middle_bb,
     std::swap (arg0, arg1);
 
   tree type = TREE_TYPE (gimple_phi_result (phi));
-  result = gimple_simplify (COND_EXPR, type,
-			    cond,
-			    arg0, arg1,
-			    &seq, NULL);
+  result = gimple_simplify_phiopt (early_p, type, stmt,
+				   arg0, arg1,
+				   &seq);
   if (!result)
     return false;
 
-- 
2.27.0

Re: [PATCH 4/7] Allow match-and-simplified phiopt to run in early phiopt

[Jeff Law]

On 6/23/2021 4:19 PM, apinski--- via Gcc-patches wrote:
> From: Andrew Pinski <apinski@marvell.com>
>
> To move a few things more to match-and-simplify from phiopt,
> we need to allow match_simplify_replacement to run in early
> phiopt. To do this we add a replacement for gimple_simplify
> that is explictly for phiopt.
>
> OK? Bootstrapped and tested on x86_64-linux-gnu with no
> regressions.
>
> gcc/ChangeLog:
>
> 	* tree-ssa-phiopt.c (match_simplify_replacement):
> 	Add early_p argument. Call gimple_simplify_phiopt
> 	instead of gimple_simplify.
> 	(tree_ssa_phiopt_worker): Update call to
> 	match_simplify_replacement and allow unconditionally.
> 	(phiopt_early_allow): New function.
> 	(gimple_simplify_phiopt): New function.
So the two questions on my end are why did we restrict when this could 
run before and why restrict the codes we're willing to optimize in the 
early phase?  Not an ACK or NAK at this point, just trying to understand 
a bit of the history.

jeff

Re: [PATCH 4/7] Allow match-and-simplified phiopt to run in early phiopt

[Richard Biener]

On Thu, Jun 24, 2021 at 6:24 PM Jeff Law via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
>
>
> On 6/23/2021 4:19 PM, apinski--- via Gcc-patches wrote:
> > From: Andrew Pinski <apinski@marvell.com>
> >
> > To move a few things more to match-and-simplify from phiopt,
> > we need to allow match_simplify_replacement to run in early
> > phiopt. To do this we add a replacement for gimple_simplify
> > that is explictly for phiopt.
> >
> > OK? Bootstrapped and tested on x86_64-linux-gnu with no
> > regressions.
> >
> > gcc/ChangeLog:
> >
> >       * tree-ssa-phiopt.c (match_simplify_replacement):
> >       Add early_p argument. Call gimple_simplify_phiopt
> >       instead of gimple_simplify.
> >       (tree_ssa_phiopt_worker): Update call to
> >       match_simplify_replacement and allow unconditionally.
> >       (phiopt_early_allow): New function.
> >       (gimple_simplify_phiopt): New function.
> So the two questions on my end are why did we restrict when this could
> run before and why restrict the codes we're willing to optimize in the
> early phase?  Not an ACK or NAK at this point, just trying to understand
> a bit of the history.

I've done this because jump threading likes to see the CFG structure
and some of the testcases use if () return 0/1 which are prone to be
value-replaced by phiopt.  At the point I added the early phiopt I
didn't want to go and fixup all the testcases to avoid the phiopt transforms
nor did I want to investigate the "real" impact on code - the purpose
of early phiopt was exactly to get min/max/abs replacement done so
that was the way of least resistance ...

I'd rather not revisit this decision as part of the match-and-simplify
series but of course if anyone dares to do the detective work she'll be
welcome.

Richard.

>
> jeff
>

Re: [PATCH 4/7] Allow match-and-simplified phiopt to run in early phiopt

[Jeff Law]

On 6/25/2021 2:24 AM, Richard Biener wrote:
> On Thu, Jun 24, 2021 at 6:24 PM Jeff Law via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>>
>>
>> On 6/23/2021 4:19 PM, apinski--- via Gcc-patches wrote:
>>> From: Andrew Pinski <apinski@marvell.com>
>>>
>>> To move a few things more to match-and-simplify from phiopt,
>>> we need to allow match_simplify_replacement to run in early
>>> phiopt. To do this we add a replacement for gimple_simplify
>>> that is explictly for phiopt.
>>>
>>> OK? Bootstrapped and tested on x86_64-linux-gnu with no
>>> regressions.
>>>
>>> gcc/ChangeLog:
>>>
>>>        * tree-ssa-phiopt.c (match_simplify_replacement):
>>>        Add early_p argument. Call gimple_simplify_phiopt
>>>        instead of gimple_simplify.
>>>        (tree_ssa_phiopt_worker): Update call to
>>>        match_simplify_replacement and allow unconditionally.
>>>        (phiopt_early_allow): New function.
>>>        (gimple_simplify_phiopt): New function.
>> So the two questions on my end are why did we restrict when this could
>> run before and why restrict the codes we're willing to optimize in the
>> early phase?  Not an ACK or NAK at this point, just trying to understand
>> a bit of the history.
> I've done this because jump threading likes to see the CFG structure
> and some of the testcases use if () return 0/1 which are prone to be
> value-replaced by phiopt.  At the point I added the early phiopt I
> didn't want to go and fixup all the testcases to avoid the phiopt transforms
> nor did I want to investigate the "real" impact on code - the purpose
> of early phiopt was exactly to get min/max/abs replacement done so
> that was the way of least resistance ...
>
> I'd rather not revisit this decision as part of the match-and-simplify
> series but of course if anyone dares to do the detective work she'll be
> welcome.
Thanks for the background.   So Andrew is largely just preserving this 
property.  Works for me.   And just to be explicit 4/7 is fine for the 
trunk.

jeff

[PATCH 5/7] Try inverted comparison for match_simplify in phiopt

[apinski]

From: Andrew Pinski <apinski@marvell.com>

Since match and simplify does not have all of the inverted
comparison patterns, it make sense to just have
phi-opt try to do the inversion and try match and simplify again.

OK? Bootstrapped and tested on x86_64-linux-gnu.

Thanks,
Andrew Pinski

gcc/ChangeLog:

	* tree-ssa-phiopt.c (gimple_simplify_phiopt):
	If "A ? B : C" fails to simplify, try "(!A) ? C : B".
---
 gcc/tree-ssa-phiopt.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c
index 8b0e68c1e90..5f099eca343 100644
--- a/gcc/tree-ssa-phiopt.c
+++ b/gcc/tree-ssa-phiopt.c
@@ -828,7 +828,8 @@ phiopt_early_allow (enum tree_code code)
    with parts pushed if EARLY_P was true. Also rejects non allowed tree code
    if EARLY_P is set.
    Takes the comparison from COMP_STMT and two args, ARG0 and ARG1 and tries
-   to simplify CMP ? ARG0 : ARG1.  */
+   to simplify CMP ? ARG0 : ARG1.
+   Also try to simplify (!CMP) ? ARG0 : ARG1 if the non-inverse failed.  */
 static tree
 gimple_simplify_phiopt (bool early_p, tree type, gimple *comp_stmt,
 			tree arg0, tree arg1,
@@ -861,6 +862,30 @@ gimple_simplify_phiopt (bool early_p, tree type, gimple *comp_stmt,
 	    return result;
 	}
     }
+  /* Try the inverted comparison, that is !COMP ? ARG1 : ARG0. */
+  comp_code = invert_tree_comparison (comp_code, HONOR_NANS (cmp0));
+
+  if (comp_code == ERROR_MARK)
+    return NULL;
+
+  cond = build2_loc (loc,
+		     comp_code, boolean_type_node,
+		     cmp0, cmp1);
+  gimple_match_op op1 (gimple_match_cond::UNCOND,
+		       COND_EXPR, type, cond, arg1, arg0);
+
+  if (op1.resimplify (early_p ? NULL : seq, follow_all_ssa_edges))
+    {
+      /* Early we want only to allow some generated tree codes. */
+      if (!early_p
+	  || op1.code.is_tree_code ()
+	  || phiopt_early_allow ((tree_code)op1.code))
+	{
+	  result = maybe_push_res_to_seq (&op1, seq);
+	  if (result)
+	    return result;
+	}
+    }
 
   return NULL;
 }
-- 
2.27.0

Re: [PATCH 5/7] Try inverted comparison for match_simplify in phiopt

[Bernhard Reutner-Fischer]

Hi Andrew,

just a nit..

On Wed, 23 Jun 2021 15:19:13 -0700
apinski--- via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:

> From: Andrew Pinski <apinski@marvell.com>
> 
> Since match and simplify does not have all of the inverted
> comparison patterns, it make sense to just have
> phi-opt try to do the inversion and try match and simplify again.
> 
> OK? Bootstrapped and tested on x86_64-linux-gnu.
> 
> Thanks,
> Andrew Pinski
> 
> gcc/ChangeLog:
> 
> 	* tree-ssa-phiopt.c (gimple_simplify_phiopt):
> 	If "A ? B : C" fails to simplify, try "(!A) ? C : B".
> ---
>  gcc/tree-ssa-phiopt.c | 27 ++++++++++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c
> index 8b0e68c1e90..5f099eca343 100644
> --- a/gcc/tree-ssa-phiopt.c
> +++ b/gcc/tree-ssa-phiopt.c
> @@ -828,7 +828,8 @@ phiopt_early_allow (enum tree_code code)
>     with parts pushed if EARLY_P was true. Also rejects non allowed tree code
>     if EARLY_P is set.
>     Takes the comparison from COMP_STMT and two args, ARG0 and ARG1 and tries
> -   to simplify CMP ? ARG0 : ARG1.  */
> +   to simplify CMP ? ARG0 : ARG1.
> +   Also try to simplify (!CMP) ? ARG0 : ARG1 if the non-inverse failed.  */

commentary typo above, args need swap:
+   Also try to simplify (!CMP) ? ARG1 : ARG0 if the non-inverse failed.  */

thanks,

Re: [PATCH 5/7] Try inverted comparison for match_simplify in phiopt

[Jeff Law]

On 6/23/2021 4:19 PM, apinski--- via Gcc-patches wrote:
> From: Andrew Pinski <apinski@marvell.com>
>
> Since match and simplify does not have all of the inverted
> comparison patterns, it make sense to just have
> phi-opt try to do the inversion and try match and simplify again.
>
> OK? Bootstrapped and tested on x86_64-linux-gnu.
>
> Thanks,
> Andrew Pinski
>
> gcc/ChangeLog:
>
> 	* tree-ssa-phiopt.c (gimple_simplify_phiopt):
> 	If "A ? B : C" fails to simplify, try "(!A) ? C : B".
OK
jeff

[PATCH 6/7] Lower for loops before lowering cond in genmatch

[apinski]

From: Andrew Pinski <apinski@marvell.com>

While converting some fold_cond_expr_with_comparison
to match, I found that I wanted to use "for cnd (cond vec_cond)"
but that was not causing the lowering of cond to happen.
What was happening was the lowering of the for loop
was happening after the lowering of the cond. So
swapping was the correct thing to do but it also
means we need to copy for_subst_vec in lower_cond.

OK?  Bootstrapped and tested on x86_64-linux-gnu with no regressions.

gcc/ChangeLog:

	* genmatch.c (lower_cond): Copy for_subst_vec
	for the simplify also.
	(lower): Swap the order for lower_for and lower_cond.
---
 gcc/genmatch.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/gcc/genmatch.c b/gcc/genmatch.c
index 4d476720c9e..970a2ebba68 100644
--- a/gcc/genmatch.c
+++ b/gcc/genmatch.c
@@ -1306,6 +1306,7 @@ lower_cond (simplify *s, vec<simplify *>& simplifiers)
     {
       simplify *ns = new simplify (s->kind, s->id, matchers[i], s->result,
 				   s->for_vec, s->capture_ids);
+      ns->for_subst_vec.safe_splice (s->for_subst_vec);
       simplifiers.safe_push (ns);
     }
 }
@@ -1543,24 +1544,27 @@ static void
 lower (vec<simplify *>& simplifiers, bool gimple)
 {
   auto_vec<simplify *> out_simplifiers;
-  for (unsigned i = 0; i < simplifiers.length (); ++i)
-    lower_opt (simplifiers[i], out_simplifiers);
+  for (auto s: simplifiers)
+    lower_opt (s, out_simplifiers);
 
   simplifiers.truncate (0);
-  for (unsigned i = 0; i < out_simplifiers.length (); ++i)
-    lower_commutative (out_simplifiers[i], simplifiers);
+  for (auto s: out_simplifiers)
+    lower_commutative (s, simplifiers);
 
+  /* Lower for needs to happen before lowering cond
+     to support (for cnd (cond vec_cond)).  This is
+     safe as substitution delay does not happen for
+     cond or vec_cond. */
   out_simplifiers.truncate (0);
-  if (gimple)
-    for (unsigned i = 0; i < simplifiers.length (); ++i)
-      lower_cond (simplifiers[i], out_simplifiers);
-  else
-    out_simplifiers.safe_splice (simplifiers);
-
+  for (auto s: simplifiers)
+    lower_for (s, out_simplifiers);
 
   simplifiers.truncate (0);
-  for (unsigned i = 0; i < out_simplifiers.length (); ++i)
-    lower_for (out_simplifiers[i], simplifiers);
+  if (gimple)
+    for (auto s: out_simplifiers)
+      lower_cond (s, simplifiers);
+  else
+    simplifiers.safe_splice (out_simplifiers);
 }
 
 
-- 
2.27.0

Re: [PATCH 6/7] Lower for loops before lowering cond in genmatch

[Jeff Law]

On 6/23/2021 4:19 PM, apinski--- via Gcc-patches wrote:
> From: Andrew Pinski <apinski@marvell.com>
>
> While converting some fold_cond_expr_with_comparison
> to match, I found that I wanted to use "for cnd (cond vec_cond)"
> but that was not causing the lowering of cond to happen.
> What was happening was the lowering of the for loop
> was happening after the lowering of the cond. So
> swapping was the correct thing to do but it also
> means we need to copy for_subst_vec in lower_cond.
>
> OK?  Bootstrapped and tested on x86_64-linux-gnu with no regressions.
>
> gcc/ChangeLog:
>
> 	* genmatch.c (lower_cond): Copy for_subst_vec
> 	for the simplify also.
> 	(lower): Swap the order for lower_for and lower_cond.
I need to defer this to Richi.  I don't know the genmatch code at all.

jeff

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

[apinski]

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.

gcc/ChangeLog:

	* 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:

	* gcc.dg/tree-ssa/phi-opt-15.c: Update test to expect
	ABSU and still not expect ABS_EXPR.
---
 gcc/match.pd                               |  60 +++++++++
 gcc/testsuite/gcc.dg/tree-ssa/phi-opt-15.c |   4 +-
 gcc/tree-ssa-phiopt.c                      | 134 +--------------------
 3 files changed, 64 insertions(+), 134 deletions(-)

diff --git a/gcc/match.pd b/gcc/match.pd
index 39fb57ee1f4..0c790dfa741 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 (element_mode (type)))
+     @1))
+  (simplify
+   (cnd (cmp @0 zerop) zerop (negate@1 @0))
+    (if (!HONOR_SIGNED_ZEROS (element_mode (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 (element_mode (type)))
+     @0))
+  (simplify
+   (cnd (cmp @0 zerop) @0 zerop)
+    (if (!HONOR_SIGNED_ZEROS (element_mode (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 (element_mode (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 (element_mode (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/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c
index 5f099eca343..cd582d08dfc 100644
--- a/gcc/tree-ssa-phiopt.c
+++ b/gcc/tree-ssa-phiopt.c
@@ -63,8 +63,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,
@@ -350,8 +348,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,
@@ -2598,134 +2594,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
@@ -3659,7 +3527,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

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

[Jeff Law]

On 6/23/2021 4:19 PM, apinski--- via Gcc-patches 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.
>
> gcc/ChangeLog:
>
> 	* 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:
>
> 	* gcc.dg/tree-ssa/phi-opt-15.c: Update test to expect
> 	ABSU and still not expect ABS_EXPR.
And I've already ack'd this part :-)  I think it's unchanged since he 
original.

Jeff