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

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

[HAO CHEN GUI]

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"
@@ -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 ())
+    {
+      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)

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

[Richard Sandiford]

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)

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

[HAO CHEN GUI]

Hi Richard,

在 2024/6/20 16:05, Richard Sandiford 写道:
> 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.

Your guess is correct.
Actually I found very complex debug insns in dump of fwprop pass if
gcc doesn't check profitability for debug insns. The compiling of
"md5.c" or "sha1.c" can't complete within one night. So I think we
should check profitability for debug insns also.

I will fix the formatting and submit the patch. Thanks again for your
review comments.

Thanks
Gui Haochen