[#3/7] introduce ifcombine_replace_cond (was: Re: [PATCH] fold fold_truth_andor field merging into ifcombine)

Message ID oro738v1ps.fsf_-_@lxoliva.fsfla.org
State New
Headers
Series [#1/7] allow vuses in ifcombine blocks (was: Re: [PATCH] fold fold_truth_andor field merging into ifcombine) |

Commit Message

Alexandre Oliva Oct. 25, 2024, 11:52 a.m. UTC
  Refactor ifcombine_ifandif, moving the common code from the various
paths that apply the combined condition to a new function.


for  gcc/ChangeLog

	* tree-ssa-ifcombine.cc (ifcombine_replace_cond): Factor out
	of...
	(ifcombine_ifandif): ... this.
---
 gcc/tree-ssa-ifcombine.cc |  137 +++++++++++++++++++++------------------------
 1 file changed, 65 insertions(+), 72 deletions(-)
  

Comments

Jeff Law Oct. 27, 2024, 8:16 p.m. UTC | #1
On 10/25/24 5:52 AM, Alexandre Oliva wrote:
> 
> Refactor ifcombine_ifandif, moving the common code from the various
> paths that apply the combined condition to a new function.
> 
> 
> for  gcc/ChangeLog
> 
> 	* tree-ssa-ifcombine.cc (ifcombine_replace_cond): Factor out
> 	of...
> 	(ifcombine_ifandif): ... this.
It looks like you also did some simplifications in ifcombine_ifandif. 
Those should be noted in the ChangeLog.  Specifically you no longer make 
the calls to force_gimple_operand_gsi and simplified the equality test.

OK with that change.

jeff
  
Richard Biener Oct. 30, 2024, 1:52 p.m. UTC | #2
On Fri, Oct 25, 2024 at 4:39 PM Alexandre Oliva <oliva@adacore.com> wrote:
>
>
> Refactor ifcombine_ifandif, moving the common code from the various
> paths that apply the combined condition to a new function.
>
>
> for  gcc/ChangeLog
>
>         * tree-ssa-ifcombine.cc (ifcombine_replace_cond): Factor out
>         of...
>         (ifcombine_ifandif): ... this.
> ---
>  gcc/tree-ssa-ifcombine.cc |  137 +++++++++++++++++++++------------------------
>  1 file changed, 65 insertions(+), 72 deletions(-)
>
> diff --git a/gcc/tree-ssa-ifcombine.cc b/gcc/tree-ssa-ifcombine.cc
> index 0a2ba970548c8..6dcf5e6efe1de 100644
> --- a/gcc/tree-ssa-ifcombine.cc
> +++ b/gcc/tree-ssa-ifcombine.cc
> @@ -399,6 +399,51 @@ update_profile_after_ifcombine (basic_block inner_cond_bb,
>    outer2->probability = profile_probability::never ();
>  }
>
> +/* Replace the conditions in INNER_COND with COND.
> +   Replace OUTER_COND with a constant.  */
> +
> +static bool
> +ifcombine_replace_cond (gcond *inner_cond, bool inner_inv,
> +                       gcond *outer_cond, bool outer_inv,
> +                       tree cond, bool must_canon, tree cond2)
> +{
> +  bool result_inv = inner_inv;
> +
> +  gcc_checking_assert (!cond2);
> +
> +  if (result_inv)
> +    cond = fold_build1 (TRUTH_NOT_EXPR, TREE_TYPE (cond), cond);
> +
> +  if (tree tcanon = canonicalize_cond_expr_cond (cond))
> +    cond = tcanon;
> +  else if (must_canon)
> +    return false;
> +
> +    {

no need for this brace pair?

OK with it dropped.

Richard.

> +      if (!is_gimple_condexpr_for_cond (cond))
> +       {
> +         gimple_stmt_iterator gsi = gsi_for_stmt (inner_cond);
> +         cond = force_gimple_operand_gsi_1 (&gsi, cond,
> +                                            is_gimple_condexpr_for_cond,
> +                                            NULL, true, GSI_SAME_STMT);
> +       }
> +      gimple_cond_set_condition_from_tree (inner_cond, cond);
> +      update_stmt (inner_cond);
> +
> +      /* Leave CFG optimization to cfg_cleanup.  */
> +      gimple_cond_set_condition_from_tree (outer_cond,
> +                                          outer_inv
> +                                          ? boolean_false_node
> +                                          : boolean_true_node);
> +      update_stmt (outer_cond);
> +    }
> +
> +  update_profile_after_ifcombine (gimple_bb (inner_cond),
> +                                 gimple_bb (outer_cond));
> +
> +  return true;
> +}
> +
>  /* If-convert on a and pattern with a common else block.  The inner
>     if is specified by its INNER_COND_BB, the outer by OUTER_COND_BB.
>     inner_inv, outer_inv indicate whether the conditions are inverted.
> @@ -408,7 +453,6 @@ static bool
>  ifcombine_ifandif (basic_block inner_cond_bb, bool inner_inv,
>                    basic_block outer_cond_bb, bool outer_inv)
>  {
> -  bool result_inv = inner_inv;
>    gimple_stmt_iterator gsi;
>    tree name1, name2, bit1, bit2, bits1, bits2;
>
> @@ -446,26 +490,13 @@ ifcombine_ifandif (basic_block inner_cond_bb, bool inner_inv,
>        t2 = fold_build2 (BIT_AND_EXPR, TREE_TYPE (name1), name1, t);
>        t2 = force_gimple_operand_gsi (&gsi, t2, true, NULL_TREE,
>                                      true, GSI_SAME_STMT);
> -      t = fold_build2 (result_inv ? NE_EXPR : EQ_EXPR,
> -                      boolean_type_node, t2, t);
> -      t = canonicalize_cond_expr_cond (t);
> -      if (!t)
> -       return false;
> -      if (!is_gimple_condexpr_for_cond (t))
> -       {
> -         gsi = gsi_for_stmt (inner_cond);
> -         t = force_gimple_operand_gsi_1 (&gsi, t, is_gimple_condexpr_for_cond,
> -                                         NULL, true, GSI_SAME_STMT);
> -       }
> -      gimple_cond_set_condition_from_tree (inner_cond, t);
> -      update_stmt (inner_cond);
>
> -      /* Leave CFG optimization to cfg_cleanup.  */
> -      gimple_cond_set_condition_from_tree (outer_cond,
> -       outer_inv ? boolean_false_node : boolean_true_node);
> -      update_stmt (outer_cond);
> +      t = fold_build2 (EQ_EXPR, boolean_type_node, t2, t);
>
> -      update_profile_after_ifcombine (inner_cond_bb, outer_cond_bb);
> +      if (!ifcombine_replace_cond (inner_cond, inner_inv,
> +                                  outer_cond, outer_inv,
> +                                  t, true, NULL_TREE))
> +       return false;
>
>        if (dump_file)
>         {
> @@ -485,9 +516,8 @@ ifcombine_ifandif (basic_block inner_cond_bb, bool inner_inv,
>       In that case remove the outer test and change the inner one to
>       test for name & (bits1 | bits2) != 0.  */
>    else if (recognize_bits_test (inner_cond, &name1, &bits1, !inner_inv)
> -      && recognize_bits_test (outer_cond, &name2, &bits2, !outer_inv))
> +          && recognize_bits_test (outer_cond, &name2, &bits2, !outer_inv))
>      {
> -      gimple_stmt_iterator gsi;
>        tree t;
>
>        if ((TREE_CODE (name1) == SSA_NAME
> @@ -530,33 +560,14 @@ ifcombine_ifandif (basic_block inner_cond_bb, bool inner_inv,
>           bits1 = fold_convert (TREE_TYPE (bits2), bits1);
>         }
>
> -      /* Do it.  */
> -      gsi = gsi_for_stmt (inner_cond);
>        t = fold_build2 (BIT_IOR_EXPR, TREE_TYPE (name1), bits1, bits2);
> -      t = force_gimple_operand_gsi (&gsi, t, true, NULL_TREE,
> -                                   true, GSI_SAME_STMT);
>        t = fold_build2 (BIT_AND_EXPR, TREE_TYPE (name1), name1, t);
> -      t = force_gimple_operand_gsi (&gsi, t, true, NULL_TREE,
> -                                   true, GSI_SAME_STMT);
> -      t = fold_build2 (result_inv ? NE_EXPR : EQ_EXPR, boolean_type_node, t,
> +      t = fold_build2 (EQ_EXPR, boolean_type_node, t,
>                        build_int_cst (TREE_TYPE (t), 0));
> -      t = canonicalize_cond_expr_cond (t);
> -      if (!t)
> +      if (!ifcombine_replace_cond (inner_cond, inner_inv,
> +                                  outer_cond, outer_inv,
> +                                  t, false, NULL_TREE))
>         return false;
> -      if (!is_gimple_condexpr_for_cond (t))
> -       {
> -         gsi = gsi_for_stmt (inner_cond);
> -         t = force_gimple_operand_gsi_1 (&gsi, t, is_gimple_condexpr_for_cond,
> -                                         NULL, true, GSI_SAME_STMT);
> -       }
> -      gimple_cond_set_condition_from_tree (inner_cond, t);
> -      update_stmt (inner_cond);
> -
> -      /* Leave CFG optimization to cfg_cleanup.  */
> -      gimple_cond_set_condition_from_tree (outer_cond,
> -       outer_inv ? boolean_false_node : boolean_true_node);
> -      update_stmt (outer_cond);
> -      update_profile_after_ifcombine (inner_cond_bb, outer_cond_bb);
>
>        if (dump_file)
>         {
> @@ -576,7 +587,7 @@ ifcombine_ifandif (basic_block inner_cond_bb, bool inner_inv,
>    else if (TREE_CODE_CLASS (gimple_cond_code (inner_cond)) == tcc_comparison
>            && TREE_CODE_CLASS (gimple_cond_code (outer_cond)) == tcc_comparison)
>      {
> -      tree t;
> +      tree t, ts = NULL_TREE;
>        enum tree_code inner_cond_code = gimple_cond_code (inner_cond);
>        enum tree_code outer_cond_code = gimple_cond_code (outer_cond);
>
> @@ -602,7 +613,6 @@ ifcombine_ifandif (basic_block inner_cond_bb, bool inner_inv,
>                                             gimple_bb (outer_cond))))
>         {
>           tree t1, t2;
> -         gimple_stmt_iterator gsi;
>           bool logical_op_non_short_circuit = LOGICAL_OP_NON_SHORT_CIRCUIT;
>           if (param_logical_op_non_short_circuit != -1)
>             logical_op_non_short_circuit
> @@ -624,39 +634,22 @@ ifcombine_ifandif (basic_block inner_cond_bb, bool inner_inv,
>                                 gimple_cond_rhs (outer_cond));
>           t = fold_build2_loc (gimple_location (inner_cond),
>                                TRUTH_AND_EXPR, boolean_type_node, t1, t2);
> -         if (result_inv)
> -           {
> -             t = fold_build1 (TRUTH_NOT_EXPR, TREE_TYPE (t), t);
> -             result_inv = false;
> -           }
> -         gsi = gsi_for_stmt (inner_cond);
> -         t = force_gimple_operand_gsi_1 (&gsi, t, is_gimple_condexpr_for_cond,
> -                                         NULL, true, GSI_SAME_STMT);
>          }
> -      if (result_inv)
> -       t = fold_build1 (TRUTH_NOT_EXPR, TREE_TYPE (t), t);
> -      t = canonicalize_cond_expr_cond (t);
> -      if (!t)
> -       return false;
> -      if (!is_gimple_condexpr_for_cond (t))
> -       {
> -         gsi = gsi_for_stmt (inner_cond);
> -         t = force_gimple_operand_gsi_1 (&gsi, t, is_gimple_condexpr_for_cond,
> -                                         NULL, true, GSI_SAME_STMT);
> -       }
> -      gimple_cond_set_condition_from_tree (inner_cond, t);
> -      update_stmt (inner_cond);
>
> -      /* Leave CFG optimization to cfg_cleanup.  */
> -      gimple_cond_set_condition_from_tree (outer_cond,
> -       outer_inv ? boolean_false_node : boolean_true_node);
> -      update_stmt (outer_cond);
> -      update_profile_after_ifcombine (inner_cond_bb, outer_cond_bb);
> +      if (!ifcombine_replace_cond (inner_cond, inner_inv,
> +                                  outer_cond, outer_inv,
> +                                  t, false, ts))
> +       return false;
>
>        if (dump_file)
>         {
>           fprintf (dump_file, "optimizing two comparisons to ");
>           print_generic_expr (dump_file, t);
> +         if (ts)
> +           {
> +             fprintf (dump_file, " and ");
> +             print_generic_expr (dump_file, ts);
> +           }
>           fprintf (dump_file, "\n");
>         }
>
>
> --
> Alexandre Oliva, happy hacker            https://FSFLA.org/blogs/lxo/
>    Free Software Activist                   GNU Toolchain Engineer
> More tolerance and less prejudice are key for inclusion and diversity
> Excluding neuro-others for not behaving ""normal"" is *not* inclusive
  
Alexandre Oliva Nov. 2, 2024, 2:30 a.m. UTC | #3
On Oct 27, 2024, Jeff Law <jeffreyalaw@gmail.com> wrote:

> On 10/25/24 5:52 AM, Alexandre Oliva wrote:
>> Refactor ifcombine_ifandif, moving the common code from the various
>> paths that apply the combined condition to a new function.
>> 
>> for  gcc/ChangeLog
>> * tree-ssa-ifcombine.cc (ifcombine_replace_cond): Factor out
>> of...
>> (ifcombine_ifandif): ... this.
> It looks like you also did some simplifications in
> ifcombine_ifandif. Those should be noted in the ChangeLog.

Indeed, thanks!

> Specifically you no longer make the calls to force_gimple_operand_gsi

True, they're now implied by the other force_gimple_operand_gsi_1 call.

> and simplified the equality test.

I'm not sure what you mean here.  Is it about leaving the inversion for
the refactored function to perform?

If so, this addition should do:

	(ifcombine_ifandif): ... this.  Leave it for the above to
	gimplify and invert the condition.
  
Alexandre Oliva Nov. 2, 2024, 2:31 a.m. UTC | #4
On Oct 30, 2024, Richard Biener <richard.guenther@gmail.com> wrote:

> no need for this brace pair?

No need indeed, but this bit becomes the else branch of an if introduced
in #5.  I figured the #5 patch would be cleaner if I kept this bit
braced and indented.
  
Sam James Nov. 2, 2024, 5:12 a.m. UTC | #5
Alexandre Oliva <oliva@adacore.com> writes:

> Refactor ifcombine_ifandif, moving the common code from the various
> paths that apply the combined condition to a new function.

BTW, forgive the possibly silly question, but I don't see any testcases
for the series. Would it be possible to add any?

>
>
> for  gcc/ChangeLog
>
> 	* tree-ssa-ifcombine.cc (ifcombine_replace_cond): Factor out
> 	of...
> 	(ifcombine_ifandif): ... this.
> ---
>  gcc/tree-ssa-ifcombine.cc |  137 +++++++++++++++++++++------------------------
>  1 file changed, 65 insertions(+), 72 deletions(-)
>
> diff --git a/gcc/tree-ssa-ifcombine.cc b/gcc/tree-ssa-ifcombine.cc
> index 0a2ba970548c8..6dcf5e6efe1de 100644
> --- a/gcc/tree-ssa-ifcombine.cc
> +++ b/gcc/tree-ssa-ifcombine.cc
> @@ -399,6 +399,51 @@ update_profile_after_ifcombine (basic_block inner_cond_bb,
>    outer2->probability = profile_probability::never ();
>  }
>  
> +/* Replace the conditions in INNER_COND with COND.
> +   Replace OUTER_COND with a constant.  */
> +
> +static bool
> +ifcombine_replace_cond (gcond *inner_cond, bool inner_inv,
> +			gcond *outer_cond, bool outer_inv,
> +			tree cond, bool must_canon, tree cond2)
> +{
> +  bool result_inv = inner_inv;
> +
> +  gcc_checking_assert (!cond2);
> +
> +  if (result_inv)
> +    cond = fold_build1 (TRUTH_NOT_EXPR, TREE_TYPE (cond), cond);
> +
> +  if (tree tcanon = canonicalize_cond_expr_cond (cond))
> +    cond = tcanon;
> +  else if (must_canon)
> +    return false;
> +
> +    {
> +      if (!is_gimple_condexpr_for_cond (cond))
> +	{
> +	  gimple_stmt_iterator gsi = gsi_for_stmt (inner_cond);
> +	  cond = force_gimple_operand_gsi_1 (&gsi, cond,
> +					     is_gimple_condexpr_for_cond,
> +					     NULL, true, GSI_SAME_STMT);
> +	}
> +      gimple_cond_set_condition_from_tree (inner_cond, cond);
> +      update_stmt (inner_cond);
> +
> +      /* Leave CFG optimization to cfg_cleanup.  */
> +      gimple_cond_set_condition_from_tree (outer_cond,
> +					   outer_inv
> +					   ? boolean_false_node
> +					   : boolean_true_node);
> +      update_stmt (outer_cond);
> +    }
> +
> +  update_profile_after_ifcombine (gimple_bb (inner_cond),
> +				  gimple_bb (outer_cond));
> +
> +  return true;
> +}
> +
>  /* If-convert on a and pattern with a common else block.  The inner
>     if is specified by its INNER_COND_BB, the outer by OUTER_COND_BB.
>     inner_inv, outer_inv indicate whether the conditions are inverted.
> @@ -408,7 +453,6 @@ static bool
>  ifcombine_ifandif (basic_block inner_cond_bb, bool inner_inv,
>  		   basic_block outer_cond_bb, bool outer_inv)
>  {
> -  bool result_inv = inner_inv;
>    gimple_stmt_iterator gsi;
>    tree name1, name2, bit1, bit2, bits1, bits2;
>  
> @@ -446,26 +490,13 @@ ifcombine_ifandif (basic_block inner_cond_bb, bool inner_inv,
>        t2 = fold_build2 (BIT_AND_EXPR, TREE_TYPE (name1), name1, t);
>        t2 = force_gimple_operand_gsi (&gsi, t2, true, NULL_TREE,
>  				     true, GSI_SAME_STMT);
> -      t = fold_build2 (result_inv ? NE_EXPR : EQ_EXPR,
> -		       boolean_type_node, t2, t);
> -      t = canonicalize_cond_expr_cond (t);
> -      if (!t)
> -	return false;
> -      if (!is_gimple_condexpr_for_cond (t))
> -	{
> -	  gsi = gsi_for_stmt (inner_cond);
> -	  t = force_gimple_operand_gsi_1 (&gsi, t, is_gimple_condexpr_for_cond,
> -					  NULL, true, GSI_SAME_STMT);
> -	}
> -      gimple_cond_set_condition_from_tree (inner_cond, t);
> -      update_stmt (inner_cond);
>  
> -      /* Leave CFG optimization to cfg_cleanup.  */
> -      gimple_cond_set_condition_from_tree (outer_cond,
> -	outer_inv ? boolean_false_node : boolean_true_node);
> -      update_stmt (outer_cond);
> +      t = fold_build2 (EQ_EXPR, boolean_type_node, t2, t);
>  
> -      update_profile_after_ifcombine (inner_cond_bb, outer_cond_bb);
> +      if (!ifcombine_replace_cond (inner_cond, inner_inv,
> +				   outer_cond, outer_inv,
> +				   t, true, NULL_TREE))
> +	return false;
>  
>        if (dump_file)
>  	{
> @@ -485,9 +516,8 @@ ifcombine_ifandif (basic_block inner_cond_bb, bool inner_inv,
>       In that case remove the outer test and change the inner one to
>       test for name & (bits1 | bits2) != 0.  */
>    else if (recognize_bits_test (inner_cond, &name1, &bits1, !inner_inv)
> -      && recognize_bits_test (outer_cond, &name2, &bits2, !outer_inv))
> +	   && recognize_bits_test (outer_cond, &name2, &bits2, !outer_inv))
>      {
> -      gimple_stmt_iterator gsi;
>        tree t;
>  
>        if ((TREE_CODE (name1) == SSA_NAME
> @@ -530,33 +560,14 @@ ifcombine_ifandif (basic_block inner_cond_bb, bool inner_inv,
>  	  bits1 = fold_convert (TREE_TYPE (bits2), bits1);
>  	}
>  
> -      /* Do it.  */
> -      gsi = gsi_for_stmt (inner_cond);
>        t = fold_build2 (BIT_IOR_EXPR, TREE_TYPE (name1), bits1, bits2);
> -      t = force_gimple_operand_gsi (&gsi, t, true, NULL_TREE,
> -				    true, GSI_SAME_STMT);
>        t = fold_build2 (BIT_AND_EXPR, TREE_TYPE (name1), name1, t);
> -      t = force_gimple_operand_gsi (&gsi, t, true, NULL_TREE,
> -				    true, GSI_SAME_STMT);
> -      t = fold_build2 (result_inv ? NE_EXPR : EQ_EXPR, boolean_type_node, t,
> +      t = fold_build2 (EQ_EXPR, boolean_type_node, t,
>  		       build_int_cst (TREE_TYPE (t), 0));
> -      t = canonicalize_cond_expr_cond (t);
> -      if (!t)
> +      if (!ifcombine_replace_cond (inner_cond, inner_inv,
> +				   outer_cond, outer_inv,
> +				   t, false, NULL_TREE))
>  	return false;
> -      if (!is_gimple_condexpr_for_cond (t))
> -	{
> -	  gsi = gsi_for_stmt (inner_cond);
> -	  t = force_gimple_operand_gsi_1 (&gsi, t, is_gimple_condexpr_for_cond,
> -					  NULL, true, GSI_SAME_STMT);
> -	}
> -      gimple_cond_set_condition_from_tree (inner_cond, t);
> -      update_stmt (inner_cond);
> -
> -      /* Leave CFG optimization to cfg_cleanup.  */
> -      gimple_cond_set_condition_from_tree (outer_cond,
> -	outer_inv ? boolean_false_node : boolean_true_node);
> -      update_stmt (outer_cond);
> -      update_profile_after_ifcombine (inner_cond_bb, outer_cond_bb);
>  
>        if (dump_file)
>  	{
> @@ -576,7 +587,7 @@ ifcombine_ifandif (basic_block inner_cond_bb, bool inner_inv,
>    else if (TREE_CODE_CLASS (gimple_cond_code (inner_cond)) == tcc_comparison
>  	   && TREE_CODE_CLASS (gimple_cond_code (outer_cond)) == tcc_comparison)
>      {
> -      tree t;
> +      tree t, ts = NULL_TREE;
>        enum tree_code inner_cond_code = gimple_cond_code (inner_cond);
>        enum tree_code outer_cond_code = gimple_cond_code (outer_cond);
>  
> @@ -602,7 +613,6 @@ ifcombine_ifandif (basic_block inner_cond_bb, bool inner_inv,
>  					    gimple_bb (outer_cond))))
>  	{
>  	  tree t1, t2;
> -	  gimple_stmt_iterator gsi;
>  	  bool logical_op_non_short_circuit = LOGICAL_OP_NON_SHORT_CIRCUIT;
>  	  if (param_logical_op_non_short_circuit != -1)
>  	    logical_op_non_short_circuit
> @@ -624,39 +634,22 @@ ifcombine_ifandif (basic_block inner_cond_bb, bool inner_inv,
>  				gimple_cond_rhs (outer_cond));
>  	  t = fold_build2_loc (gimple_location (inner_cond), 
>  			       TRUTH_AND_EXPR, boolean_type_node, t1, t2);
> -	  if (result_inv)
> -	    {
> -	      t = fold_build1 (TRUTH_NOT_EXPR, TREE_TYPE (t), t);
> -	      result_inv = false;
> -	    }
> -	  gsi = gsi_for_stmt (inner_cond);
> -	  t = force_gimple_operand_gsi_1 (&gsi, t, is_gimple_condexpr_for_cond,
> -					  NULL, true, GSI_SAME_STMT);
>          }
> -      if (result_inv)
> -	t = fold_build1 (TRUTH_NOT_EXPR, TREE_TYPE (t), t);
> -      t = canonicalize_cond_expr_cond (t);
> -      if (!t)
> -	return false;
> -      if (!is_gimple_condexpr_for_cond (t))
> -	{
> -	  gsi = gsi_for_stmt (inner_cond);
> -	  t = force_gimple_operand_gsi_1 (&gsi, t, is_gimple_condexpr_for_cond,
> -					  NULL, true, GSI_SAME_STMT);
> -	}
> -      gimple_cond_set_condition_from_tree (inner_cond, t);
> -      update_stmt (inner_cond);
>  
> -      /* Leave CFG optimization to cfg_cleanup.  */
> -      gimple_cond_set_condition_from_tree (outer_cond,
> -	outer_inv ? boolean_false_node : boolean_true_node);
> -      update_stmt (outer_cond);
> -      update_profile_after_ifcombine (inner_cond_bb, outer_cond_bb);
> +      if (!ifcombine_replace_cond (inner_cond, inner_inv,
> +				   outer_cond, outer_inv,
> +				   t, false, ts))
> +	return false;
>  
>        if (dump_file)
>  	{
>  	  fprintf (dump_file, "optimizing two comparisons to ");
>  	  print_generic_expr (dump_file, t);
> +	  if (ts)
> +	    {
> +	      fprintf (dump_file, " and ");
> +	      print_generic_expr (dump_file, ts);
> +	    }
>  	  fprintf (dump_file, "\n");
>  	}
  
Alexandre Oliva Nov. 2, 2024, 11:23 a.m. UTC | #6
On Nov  2, 2024, Sam James <sam@gentoo.org> wrote:

> BTW, forgive the possibly silly question, but I don't see any testcases
> for the series.

They're in the original patch, upthread, that this series was split out
of at reviewers' request.  The tests all depended on a feature that was
not to be included in this first patchset, but they were there, and I'm
still testing with the feature and the tests on top of the series.

> Would it be possible to add any?

It would, but they'd very soon (hopefully) be redundant, and
bootstrapping is a pretty thorough test already.
  

Patch

diff --git a/gcc/tree-ssa-ifcombine.cc b/gcc/tree-ssa-ifcombine.cc
index 0a2ba970548c8..6dcf5e6efe1de 100644
--- a/gcc/tree-ssa-ifcombine.cc
+++ b/gcc/tree-ssa-ifcombine.cc
@@ -399,6 +399,51 @@  update_profile_after_ifcombine (basic_block inner_cond_bb,
   outer2->probability = profile_probability::never ();
 }
 
+/* Replace the conditions in INNER_COND with COND.
+   Replace OUTER_COND with a constant.  */
+
+static bool
+ifcombine_replace_cond (gcond *inner_cond, bool inner_inv,
+			gcond *outer_cond, bool outer_inv,
+			tree cond, bool must_canon, tree cond2)
+{
+  bool result_inv = inner_inv;
+
+  gcc_checking_assert (!cond2);
+
+  if (result_inv)
+    cond = fold_build1 (TRUTH_NOT_EXPR, TREE_TYPE (cond), cond);
+
+  if (tree tcanon = canonicalize_cond_expr_cond (cond))
+    cond = tcanon;
+  else if (must_canon)
+    return false;
+
+    {
+      if (!is_gimple_condexpr_for_cond (cond))
+	{
+	  gimple_stmt_iterator gsi = gsi_for_stmt (inner_cond);
+	  cond = force_gimple_operand_gsi_1 (&gsi, cond,
+					     is_gimple_condexpr_for_cond,
+					     NULL, true, GSI_SAME_STMT);
+	}
+      gimple_cond_set_condition_from_tree (inner_cond, cond);
+      update_stmt (inner_cond);
+
+      /* Leave CFG optimization to cfg_cleanup.  */
+      gimple_cond_set_condition_from_tree (outer_cond,
+					   outer_inv
+					   ? boolean_false_node
+					   : boolean_true_node);
+      update_stmt (outer_cond);
+    }
+
+  update_profile_after_ifcombine (gimple_bb (inner_cond),
+				  gimple_bb (outer_cond));
+
+  return true;
+}
+
 /* If-convert on a and pattern with a common else block.  The inner
    if is specified by its INNER_COND_BB, the outer by OUTER_COND_BB.
    inner_inv, outer_inv indicate whether the conditions are inverted.
@@ -408,7 +453,6 @@  static bool
 ifcombine_ifandif (basic_block inner_cond_bb, bool inner_inv,
 		   basic_block outer_cond_bb, bool outer_inv)
 {
-  bool result_inv = inner_inv;
   gimple_stmt_iterator gsi;
   tree name1, name2, bit1, bit2, bits1, bits2;
 
@@ -446,26 +490,13 @@  ifcombine_ifandif (basic_block inner_cond_bb, bool inner_inv,
       t2 = fold_build2 (BIT_AND_EXPR, TREE_TYPE (name1), name1, t);
       t2 = force_gimple_operand_gsi (&gsi, t2, true, NULL_TREE,
 				     true, GSI_SAME_STMT);
-      t = fold_build2 (result_inv ? NE_EXPR : EQ_EXPR,
-		       boolean_type_node, t2, t);
-      t = canonicalize_cond_expr_cond (t);
-      if (!t)
-	return false;
-      if (!is_gimple_condexpr_for_cond (t))
-	{
-	  gsi = gsi_for_stmt (inner_cond);
-	  t = force_gimple_operand_gsi_1 (&gsi, t, is_gimple_condexpr_for_cond,
-					  NULL, true, GSI_SAME_STMT);
-	}
-      gimple_cond_set_condition_from_tree (inner_cond, t);
-      update_stmt (inner_cond);
 
-      /* Leave CFG optimization to cfg_cleanup.  */
-      gimple_cond_set_condition_from_tree (outer_cond,
-	outer_inv ? boolean_false_node : boolean_true_node);
-      update_stmt (outer_cond);
+      t = fold_build2 (EQ_EXPR, boolean_type_node, t2, t);
 
-      update_profile_after_ifcombine (inner_cond_bb, outer_cond_bb);
+      if (!ifcombine_replace_cond (inner_cond, inner_inv,
+				   outer_cond, outer_inv,
+				   t, true, NULL_TREE))
+	return false;
 
       if (dump_file)
 	{
@@ -485,9 +516,8 @@  ifcombine_ifandif (basic_block inner_cond_bb, bool inner_inv,
      In that case remove the outer test and change the inner one to
      test for name & (bits1 | bits2) != 0.  */
   else if (recognize_bits_test (inner_cond, &name1, &bits1, !inner_inv)
-      && recognize_bits_test (outer_cond, &name2, &bits2, !outer_inv))
+	   && recognize_bits_test (outer_cond, &name2, &bits2, !outer_inv))
     {
-      gimple_stmt_iterator gsi;
       tree t;
 
       if ((TREE_CODE (name1) == SSA_NAME
@@ -530,33 +560,14 @@  ifcombine_ifandif (basic_block inner_cond_bb, bool inner_inv,
 	  bits1 = fold_convert (TREE_TYPE (bits2), bits1);
 	}
 
-      /* Do it.  */
-      gsi = gsi_for_stmt (inner_cond);
       t = fold_build2 (BIT_IOR_EXPR, TREE_TYPE (name1), bits1, bits2);
-      t = force_gimple_operand_gsi (&gsi, t, true, NULL_TREE,
-				    true, GSI_SAME_STMT);
       t = fold_build2 (BIT_AND_EXPR, TREE_TYPE (name1), name1, t);
-      t = force_gimple_operand_gsi (&gsi, t, true, NULL_TREE,
-				    true, GSI_SAME_STMT);
-      t = fold_build2 (result_inv ? NE_EXPR : EQ_EXPR, boolean_type_node, t,
+      t = fold_build2 (EQ_EXPR, boolean_type_node, t,
 		       build_int_cst (TREE_TYPE (t), 0));
-      t = canonicalize_cond_expr_cond (t);
-      if (!t)
+      if (!ifcombine_replace_cond (inner_cond, inner_inv,
+				   outer_cond, outer_inv,
+				   t, false, NULL_TREE))
 	return false;
-      if (!is_gimple_condexpr_for_cond (t))
-	{
-	  gsi = gsi_for_stmt (inner_cond);
-	  t = force_gimple_operand_gsi_1 (&gsi, t, is_gimple_condexpr_for_cond,
-					  NULL, true, GSI_SAME_STMT);
-	}
-      gimple_cond_set_condition_from_tree (inner_cond, t);
-      update_stmt (inner_cond);
-
-      /* Leave CFG optimization to cfg_cleanup.  */
-      gimple_cond_set_condition_from_tree (outer_cond,
-	outer_inv ? boolean_false_node : boolean_true_node);
-      update_stmt (outer_cond);
-      update_profile_after_ifcombine (inner_cond_bb, outer_cond_bb);
 
       if (dump_file)
 	{
@@ -576,7 +587,7 @@  ifcombine_ifandif (basic_block inner_cond_bb, bool inner_inv,
   else if (TREE_CODE_CLASS (gimple_cond_code (inner_cond)) == tcc_comparison
 	   && TREE_CODE_CLASS (gimple_cond_code (outer_cond)) == tcc_comparison)
     {
-      tree t;
+      tree t, ts = NULL_TREE;
       enum tree_code inner_cond_code = gimple_cond_code (inner_cond);
       enum tree_code outer_cond_code = gimple_cond_code (outer_cond);
 
@@ -602,7 +613,6 @@  ifcombine_ifandif (basic_block inner_cond_bb, bool inner_inv,
 					    gimple_bb (outer_cond))))
 	{
 	  tree t1, t2;
-	  gimple_stmt_iterator gsi;
 	  bool logical_op_non_short_circuit = LOGICAL_OP_NON_SHORT_CIRCUIT;
 	  if (param_logical_op_non_short_circuit != -1)
 	    logical_op_non_short_circuit
@@ -624,39 +634,22 @@  ifcombine_ifandif (basic_block inner_cond_bb, bool inner_inv,
 				gimple_cond_rhs (outer_cond));
 	  t = fold_build2_loc (gimple_location (inner_cond), 
 			       TRUTH_AND_EXPR, boolean_type_node, t1, t2);
-	  if (result_inv)
-	    {
-	      t = fold_build1 (TRUTH_NOT_EXPR, TREE_TYPE (t), t);
-	      result_inv = false;
-	    }
-	  gsi = gsi_for_stmt (inner_cond);
-	  t = force_gimple_operand_gsi_1 (&gsi, t, is_gimple_condexpr_for_cond,
-					  NULL, true, GSI_SAME_STMT);
         }
-      if (result_inv)
-	t = fold_build1 (TRUTH_NOT_EXPR, TREE_TYPE (t), t);
-      t = canonicalize_cond_expr_cond (t);
-      if (!t)
-	return false;
-      if (!is_gimple_condexpr_for_cond (t))
-	{
-	  gsi = gsi_for_stmt (inner_cond);
-	  t = force_gimple_operand_gsi_1 (&gsi, t, is_gimple_condexpr_for_cond,
-					  NULL, true, GSI_SAME_STMT);
-	}
-      gimple_cond_set_condition_from_tree (inner_cond, t);
-      update_stmt (inner_cond);
 
-      /* Leave CFG optimization to cfg_cleanup.  */
-      gimple_cond_set_condition_from_tree (outer_cond,
-	outer_inv ? boolean_false_node : boolean_true_node);
-      update_stmt (outer_cond);
-      update_profile_after_ifcombine (inner_cond_bb, outer_cond_bb);
+      if (!ifcombine_replace_cond (inner_cond, inner_inv,
+				   outer_cond, outer_inv,
+				   t, false, ts))
+	return false;
 
       if (dump_file)
 	{
 	  fprintf (dump_file, "optimizing two comparisons to ");
 	  print_generic_expr (dump_file, t);
+	  if (ts)
+	    {
+	      fprintf (dump_file, " and ");
+	      print_generic_expr (dump_file, ts);
+	    }
 	  fprintf (dump_file, "\n");
 	}