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

[Richard Sandiford <richard dot sandiford at arm dot com>]

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