[4/9] ifcvt: Estimate original costs before convert_multiple.

Message ID 20190802151028.15590-5-rdapp@linux.ibm.com
State New
Headers show
Series
  • Improve icvt "convert multiple"
Related show

Commit Message

Robin Dapp Aug. 2, 2019, 3:10 p.m.
This patch extends bb_ok_for_noce_convert_multiple_sets by a temporary
cost estimation that can be used by noce_convert_multiple_sets.

---
 gcc/ifcvt.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

-- 
2.17.0

Comments

Richard Sandiford Aug. 6, 2019, 8:14 p.m. | #1
Robin Dapp <rdapp@linux.ibm.com> writes:
> This patch extends bb_ok_for_noce_convert_multiple_sets by a temporary

> cost estimation that can be used by noce_convert_multiple_sets.


I agree it looks like an omission that we didn't do this.  The patch
looks OK to me (maybe independently of the rest?) bar minor things:

> diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c

> index 253b8a96c1a..55205cac153 100644

> --- a/gcc/ifcvt.c

> +++ b/gcc/ifcvt.c

> @@ -3333,11 +3333,13 @@ noce_convert_multiple_sets (struct noce_if_info *if_info)

>     fewer than PARAM_MAX_RTL_IF_CONVERSION_INSNS sets.  */

>  

>  static bool

> -bb_ok_for_noce_convert_multiple_sets (basic_block test_bb)

> +bb_ok_for_noce_convert_multiple_sets (basic_block test_bb, unsigned *cost)


The function comment should document COST.

>  {

>    rtx_insn *insn;

>    unsigned count = 0;

>    unsigned param = PARAM_VALUE (PARAM_MAX_RTL_IF_CONVERSION_INSNS);

> +  bool speed_p = optimize_bb_for_speed_p (test_bb);

> +  unsigned potential_cost = 0;

>  

>    FOR_BB_INSNS (test_bb, insn)

>      {

> @@ -3373,9 +3375,14 @@ bb_ok_for_noce_convert_multiple_sets (basic_block test_bb)

>        if (!can_conditionally_move_p (GET_MODE (dest)))

>  	return false;

>  

> +      rtx sset = single_set (insn);


This is already available as "set", unless I'm missing something.

> +      potential_cost += pattern_cost (sset, speed_p);

> +

>        count++;

>      }

>  

> +  *cost += potential_cost;

> +

>    /* If we would only put out one conditional move, the other strategies

>       this pass tries are better optimized and will be more appropriate.

>       Some targets want to strictly limit the number of conditional moves

> @@ -3414,11 +3421,15 @@ noce_process_if_block (struct noce_if_info *if_info)

>       ??? For future expansion, further expand the "multiple X" rules.  */

>  

>    /* First look for multiple SETS.  */

> +  unsigned int mcost = if_info->original_cost;

> +  unsigned tmp_cost = if_info->original_cost;


Very minor, but it'd be good to be consistent about the choice
between unsigned and unsigned int. Maybe "old_cost" would be a
better name than "tmp_cost".

>    if (!else_bb

>        && HAVE_conditional_move

>        && !HAVE_cc0

> -      && bb_ok_for_noce_convert_multiple_sets (then_bb))

> +      && bb_ok_for_noce_convert_multiple_sets (then_bb, &mcost))

>      {

> +      /* Temporarily set the original costs to what we estimated.  */

> +      if_info->original_cost = mcost;

>        if (noce_convert_multiple_sets (if_info))

>  	{

>  	  if (dump_file && if_info->transform)

> @@ -3427,6 +3438,8 @@ noce_process_if_block (struct noce_if_info *if_info)

>  	  return TRUE;

>  	}

>      }

> +  /* Restore the original costs.  */

> +  if_info->original_cost = tmp_cost;

>  

>    bool speed_p = optimize_bb_for_speed_p (test_bb);

>    unsigned int then_cost = 0, else_cost = 0;


I guess the save and restore only really need to be done inside the
outer "if".  Not that the performance difference is going to be
noticeable, but maybe it would be a bit clearer to read.

Thanks,
Richard

Patch

diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
index 253b8a96c1a..55205cac153 100644
--- a/gcc/ifcvt.c
+++ b/gcc/ifcvt.c
@@ -3333,11 +3333,13 @@  noce_convert_multiple_sets (struct noce_if_info *if_info)
    fewer than PARAM_MAX_RTL_IF_CONVERSION_INSNS sets.  */
 
 static bool
-bb_ok_for_noce_convert_multiple_sets (basic_block test_bb)
+bb_ok_for_noce_convert_multiple_sets (basic_block test_bb, unsigned *cost)
 {
   rtx_insn *insn;
   unsigned count = 0;
   unsigned param = PARAM_VALUE (PARAM_MAX_RTL_IF_CONVERSION_INSNS);
+  bool speed_p = optimize_bb_for_speed_p (test_bb);
+  unsigned potential_cost = 0;
 
   FOR_BB_INSNS (test_bb, insn)
     {
@@ -3373,9 +3375,14 @@  bb_ok_for_noce_convert_multiple_sets (basic_block test_bb)
       if (!can_conditionally_move_p (GET_MODE (dest)))
 	return false;
 
+      rtx sset = single_set (insn);
+      potential_cost += pattern_cost (sset, speed_p);
+
       count++;
     }
 
+  *cost += potential_cost;
+
   /* If we would only put out one conditional move, the other strategies
      this pass tries are better optimized and will be more appropriate.
      Some targets want to strictly limit the number of conditional moves
@@ -3414,11 +3421,15 @@  noce_process_if_block (struct noce_if_info *if_info)
      ??? For future expansion, further expand the "multiple X" rules.  */
 
   /* First look for multiple SETS.  */
+  unsigned int mcost = if_info->original_cost;
+  unsigned tmp_cost = if_info->original_cost;
   if (!else_bb
       && HAVE_conditional_move
       && !HAVE_cc0
-      && bb_ok_for_noce_convert_multiple_sets (then_bb))
+      && bb_ok_for_noce_convert_multiple_sets (then_bb, &mcost))
     {
+      /* Temporarily set the original costs to what we estimated.  */
+      if_info->original_cost = mcost;
       if (noce_convert_multiple_sets (if_info))
 	{
 	  if (dump_file && if_info->transform)
@@ -3427,6 +3438,8 @@  noce_process_if_block (struct noce_if_info *if_info)
 	  return TRUE;
 	}
     }
+  /* Restore the original costs.  */
+  if_info->original_cost = tmp_cost;
 
   bool speed_p = optimize_bb_for_speed_p (test_bb);
   unsigned int then_cost = 0, else_cost = 0;