Re: [PATCH-1v5] fwprop: Replace rtx_cost with insn_cost in try_fwprop_subst_pattern [PR113325]

[Richard Sandiford <richard.sandiford@arm.com>]

HAO CHEN GUI <guihaoc@linux.ibm.com> writes:
> Hi,
>   This patch replaces rtx_cost with insn_cost in forward propagation.
> In the PR, one constant vector should be propagated and replace a
> pseudo in a store insn if we know it's a duplicated constant vector.
> It reduces the insn cost but not rtx cost. In this case, the cost is
> determined by destination operand (memory or pseudo). Unfortunately,
> rtx cost can't help.
>
>   The test case is added in the second rs6000 specific patch.
>
>   Compared to previous version, the main changes are:
> 1. Remove !single_set at checking likely_profitable_p. Add is_debug_insn
> here, so that debug insn still need to be profitable.
> 2. Remove single_set check for cost comparison. Add !is_debug_insn here,
> so that debug insn doesn't need to check the insn cost.
>
> Previous version
> https://gcc.gnu.org/pipermail/gcc-patches/2024-June/654964.html
>
>   The patch causes a regression case on i386 as the pattern cost
> regulation has a bug. Please refer the patch and discussion here.
> https://gcc.gnu.org/pipermail/gcc-patches/2024-May/651363.html
>
>   Bootstrapped and tested on powerpc64-linux BE and LE with no
> regressions. Is it OK for the trunk?
>
> Thanks
> Gui Haochen
>
> ChangeLog
> fwprop: invoke change_is_worthwhile to judge if a replacement is worthwhile
>
> gcc/
> 	* fwprop.cc (try_fwprop_subst_pattern): Invoke change_is_worthwhile
> 	to judge if a replacement is worthwhile.  Remove single_set check
> 	and add is_debug_insn check.
> 	* recog.cc (swap_change): Invalidate recog_data when the cached INSN
> 	is swapped out.
> 	* rtl-ssa/changes.cc (rtl_ssa::changes_are_worthwhile): Check if the
> 	insn cost of new rtl is unknown and fail the replacement.
>
> patch.diff
> diff --git a/gcc/fwprop.cc b/gcc/fwprop.cc
> index de543923b92..60e3ea31edc 100644
> --- a/gcc/fwprop.cc
> +++ b/gcc/fwprop.cc
> @@ -453,7 +453,7 @@ try_fwprop_subst_pattern (obstack_watermark &attempt, insn_change &use_change,
>        && (prop.changed_mem_p ()
>  	  || contains_mem_rtx_p (src)
>  	  || use_insn->is_asm ()
> -	  || !single_set (use_rtl)))
> +	  || use_insn->is_debug_insn ()))
>      {
>        if (dump_file && (dump_flags & TDF_DETAILS))
>  	fprintf (dump_file, "cannot propagate from insn %d into"

Hmm.  It's hard to tell whether the author was deliberately excluding
debug insns here, or whether doing that was just a side-effect of
disallowing multiple sets.

I'm not sure "profitability" means much in the case of debug insns.
There again, I suppose having no limit on the complexity of debug
insns would risk quadraticness and (even without that) would tend
to increase the size of the on-disk debug info.

So I agree that the change is correct and that we should continue
to disallow debug insns here.

> @@ -471,29 +471,18 @@ try_fwprop_subst_pattern (obstack_watermark &attempt, insn_change &use_change,
>        redo_changes (0);
>      }
>
> -  /* ??? In theory, it should be better to use insn costs rather than
> -     set_src_costs here.  That would involve replacing this code with
> -     change_is_worthwhile.  */
>    bool ok = recog (attempt, use_change);
> -  if (ok && !prop.changed_mem_p () && !use_insn->is_asm ())
> -    if (rtx use_set = single_set (use_rtl))
> -      {
> -	bool speed = optimize_bb_for_speed_p (BLOCK_FOR_INSN (use_rtl));
> -	temporarily_undo_changes (0);
> -	auto old_cost = set_src_cost (SET_SRC (use_set),
> -				      GET_MODE (SET_DEST (use_set)), speed);
> -	redo_changes (0);
> -	auto new_cost = set_src_cost (SET_SRC (use_set),
> -				      GET_MODE (SET_DEST (use_set)), speed);
> -	if (new_cost > old_cost
> -	    || (new_cost == old_cost && !prop.likely_profitable_p ()))
> -	  {
> -	    if (dump_file)
> -	      fprintf (dump_file, "change not profitable"
> -		       " (cost %d -> cost %d)\n", old_cost, new_cost);
> -	    ok = false;
> -	  }
> -      }
> +  if (ok && !prop.changed_mem_p () && !use_insn->is_asm ()
> +      && !use_insn->is_debug_insn ())

Very minor nit, but now that the condition doesn't fit on a single line,
each "&&" should be on its own line:

  if (ok
      && !prop.changed_mem_p ()
      && !use_insn->is_asm ()
      && !use_insn->is_debug_insn ())

OK with that formatting change, thanks.

Richard

> +    {
> +      bool strict_p = !prop.likely_profitable_p ();
> +      if (!change_is_worthwhile (use_change, strict_p))
> +	{
> +	  if (dump_file)
> +	    fprintf (dump_file, "change not profitable");
> +	  ok = false;
> +	}
> +    }
>
>    if (!ok)
>      {
> diff --git a/gcc/recog.cc b/gcc/recog.cc
> index a6799e3f5e6..56370e40e01 100644
> --- a/gcc/recog.cc
> +++ b/gcc/recog.cc
> @@ -614,7 +614,11 @@ swap_change (int num)
>    else
>      std::swap (*changes[num].loc, changes[num].old);
>    if (changes[num].object && !MEM_P (changes[num].object))
> -    std::swap (INSN_CODE (changes[num].object), changes[num].old_code);
> +    {
> +      std::swap (INSN_CODE (changes[num].object), changes[num].old_code);
> +      if (recog_data.insn == changes[num].object)
> +	recog_data.insn = nullptr;
> +    }
>  }
>
>  /* Temporarily undo all the changes numbered NUM and up, with a view
> diff --git a/gcc/rtl-ssa/changes.cc b/gcc/rtl-ssa/changes.cc
> index 11639e81bb7..c5ac4956a19 100644
> --- a/gcc/rtl-ssa/changes.cc
> +++ b/gcc/rtl-ssa/changes.cc
> @@ -186,6 +186,14 @@ rtl_ssa::changes_are_worthwhile (array_slice<insn_change *const> changes,
>        if (!change->is_deletion ())
>  	{
>  	  change->new_cost = insn_cost (change->rtl (), for_speed);
> +	  /* If the cost is unknown, replacement is not worthwhile.  */
> +	  if (!change->new_cost)
> +	    {
> +	      if (dump_file && (dump_flags & TDF_DETAILS))
> +		fprintf (dump_file,
> +			 "Reject replacement due to unknown insn cost.\n");
> +	      return false;
> +	    }
>  	  new_cost += change->new_cost;
>  	  if (for_speed)
>  	    weighted_new_cost += (cfg_bb->count.to_sreal_scale (entry_count)