diff mbox series

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

Message ID f90ce09e-a2e5-46c9-b37c-ec046ef06e5e@linux.ibm.com
State New
Headers show
Series [PATCH-1v3] fwprop: Replace rtx_cost with insn_cost in try_fwprop_subst_pattern [PR113325] | expand

Commit Message

HAO CHEN GUI June 12, 2024, 2:41 a.m. UTC 
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. Invoke change_is_worthwhile to judge if the cost is reduced and
the replacement is worthwhile.
2. Invalidate recog data before getting the insn cost for the new
rtl as insn cost might call extract_constrain_insn_cached and
extract_insn_cached to cache the recog data. The cache data is
invalid for the new rtl and it causes ICE.
3. Check if the insn cost of new rtl is zero which means unknown
cost. The replacement should be rejected at this situation.

Previous version
https://gcc.gnu.org/pipermail/gcc-patches/2024-May/651233.html

  The patch causes a regression cases 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?

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.
	* rtl-ssa/changes.cc (rtl_ssa::changes_are_worthwhile): Invalidate
	recog data before getting the insn cost for the new rtl.  Check if
	the insn cost of new rtl is unknown and fail the replacement.

patch.diff

Comments

HAO CHEN GUI June 12, 2024, 2:52 a.m. UTC | #1 
Missing CC to Jeff Law. Sorry.

在 2024/6/12 10:41, 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. Invoke change_is_worthwhile to judge if the cost is reduced and
> the replacement is worthwhile.
> 2. Invalidate recog data before getting the insn cost for the new
> rtl as insn cost might call extract_constrain_insn_cached and
> extract_insn_cached to cache the recog data. The cache data is
> invalid for the new rtl and it causes ICE.
> 3. Check if the insn cost of new rtl is zero which means unknown
> cost. The replacement should be rejected at this situation.
> 
> Previous version
> https://gcc.gnu.org/pipermail/gcc-patches/2024-May/651233.html
> 
>   The patch causes a regression cases 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?
> 
> 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.
> 	* rtl-ssa/changes.cc (rtl_ssa::changes_are_worthwhile): Invalidate
> 	recog data before getting the insn cost for the new rtl.  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..975de0eec7f 100644
> --- a/gcc/fwprop.cc
> +++ b/gcc/fwprop.cc
> @@ -471,29 +471,19 @@ 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 ()
> +      && single_set (use_rtl))
> +    {
> +      if (!change_is_worthwhile (use_change, false)
> +	  || (!prop.likely_profitable_p ()
> +	      && !change_is_worthwhile (use_change, true)))
> +	{
> +	  if (dump_file)
> +	    fprintf (dump_file, "change not profitable");
> +	  ok = false;
> +	}
> +    }
> 
>    if (!ok)
>      {
> diff --git a/gcc/rtl-ssa/changes.cc b/gcc/rtl-ssa/changes.cc
> index 11639e81bb7..9bad6c2070c 100644
> --- a/gcc/rtl-ssa/changes.cc
> +++ b/gcc/rtl-ssa/changes.cc
> @@ -185,7 +185,18 @@ rtl_ssa::changes_are_worthwhile (array_slice<insn_change *const> changes,
>  			      * change->old_cost ());
>        if (!change->is_deletion ())
>  	{
> +	  /* Invalidate recog data as insn_cost may call
> +	     extract_insn_cached.  */
> +	  INSN_CODE (change->rtl ()) = -1;
>  	  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)
Richard Sandiford June 12, 2024, 7:51 a.m. UTC | #2 
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. Invoke change_is_worthwhile to judge if the cost is reduced and
> the replacement is worthwhile.
> 2. Invalidate recog data before getting the insn cost for the new
> rtl as insn cost might call extract_constrain_insn_cached and
> extract_insn_cached to cache the recog data. The cache data is
> invalid for the new rtl and it causes ICE.
> 3. Check if the insn cost of new rtl is zero which means unknown
> cost. The replacement should be rejected at this situation.
>
> Previous version
> https://gcc.gnu.org/pipermail/gcc-patches/2024-May/651233.html
>
>   The patch causes a regression cases 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?
>
> 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.
> 	* rtl-ssa/changes.cc (rtl_ssa::changes_are_worthwhile): Invalidate
> 	recog data before getting the insn cost for the new rtl.  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..975de0eec7f 100644
> --- a/gcc/fwprop.cc
> +++ b/gcc/fwprop.cc
> @@ -471,29 +471,19 @@ 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 ()
> +      && single_set (use_rtl))
> +    {
> +      if (!change_is_worthwhile (use_change, false)
> +	  || (!prop.likely_profitable_p ()
> +	      && !change_is_worthwhile (use_change, true)))
> +	{
> +	  if (dump_file)
> +	    fprintf (dump_file, "change not profitable");
> +	  ok = false;
> +	}
> +    }

It should only be necessary to call change_is_worthwhile once,
with strict == !prop.likely_profitable_p ()

So something like:

  bool ok = recog (attempt, use_change);
  if (ok && !prop.changed_mem_p () && !use_insn->is_asm ())
    {
      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;
	}
    }

> diff --git a/gcc/rtl-ssa/changes.cc b/gcc/rtl-ssa/changes.cc
> index 11639e81bb7..9bad6c2070c 100644
> --- a/gcc/rtl-ssa/changes.cc
> +++ b/gcc/rtl-ssa/changes.cc
> @@ -185,7 +185,18 @@ rtl_ssa::changes_are_worthwhile (array_slice<insn_change *const> changes,
>  			      * change->old_cost ());
>        if (!change->is_deletion ())
>  	{
> +	  /* Invalidate recog data as insn_cost may call
> +	     extract_insn_cached.  */
> +	  INSN_CODE (change->rtl ()) = -1;

The:

  bool ok = recog (attempt, use_change);

should leave INSN_CODE set to the result of the successful recog.
Why isn't that true in the example you hit?

I wondered whether we might be trying to cost a NOOP_MOVE_INSN_CODE,
since I couldn't see anything in the current code to stop that.
But if so, that's a bug.  NOOP_MOVE_INSN_CODE should have zero cost,
and shouldn't go through insn_cost.

Thanks,
Richard

>  	  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)
HAO CHEN GUI June 14, 2024, 7:16 a.m. UTC | #3 
Hi Richard,
  Thanks for your comments.

在 2024/6/12 15:51, Richard Sandiford 写道:
> It should only be necessary to call change_is_worthwhile once,
> with strict == !prop.likely_profitable_p ()
> 
> So something like:
> 
>   bool ok = recog (attempt, use_change);
>   if (ok && !prop.changed_mem_p () && !use_insn->is_asm ())
>     {
>       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;
> 	}
>     }

I will modify the code. Thanks.

> The:
> 
>   bool ok = recog (attempt, use_change);
> 
> should leave INSN_CODE set to the result of the successful recog.
> Why isn't that true in the example you hit?
> 
> I wondered whether we might be trying to cost a NOOP_MOVE_INSN_CODE,
> since I couldn't see anything in the current code to stop that.
> But if so, that's a bug.  NOOP_MOVE_INSN_CODE should have zero cost,
> and shouldn't go through insn_cost.

Yes, the recog sets the recog data for the new rtl. But
changes_are_worthwhile calls change->old_cost and finally calls
calculate_cost to get the insn cost for old rtl. The undo/redo_changes
don't restore/recover the recog_data, which causes insn_cost takes the
wrong number of operands (recog_data.n_operands) and wrong operands
(recog_data.operand[]). Note, rs6000_insn_cost checks if the recog data
is cached. If so, it doesn't recog the insn again.

void
insn_info::calculate_cost () const
{
  basic_block cfg_bb = BLOCK_FOR_INSN (m_rtl);
  temporarily_undo_changes (0);
  m_cost_or_uid = insn_cost (m_rtl, optimize_bb_for_speed_p (cfg_bb));
  redo_changes (0);
}

// A simple debug output for calculate_cost
before undo
INSN CODE 850
n_operands 3

after undo
INSN CODE 685
n_operands 3

Actually insn pattern 850 has 3 operands and insn pattern 685 has 2
operands.

IMHO, we should invalidate recog data after each undo/redo or before
calling the insn cost.

Looking forward to your advice.

Thanks
Gui Haochen
Richard Sandiford June 14, 2024, 8:36 a.m. UTC | #4 
HAO CHEN GUI <guihaoc@linux.ibm.com> writes:
>> The:
>> 
>>   bool ok = recog (attempt, use_change);
>> 
>> should leave INSN_CODE set to the result of the successful recog.
>> Why isn't that true in the example you hit?
>> 
>> I wondered whether we might be trying to cost a NOOP_MOVE_INSN_CODE,
>> since I couldn't see anything in the current code to stop that.
>> But if so, that's a bug.  NOOP_MOVE_INSN_CODE should have zero cost,
>> and shouldn't go through insn_cost.
>
> Yes, the recog sets the recog data for the new rtl. But
> changes_are_worthwhile calls change->old_cost and finally calls
> calculate_cost to get the insn cost for old rtl. The undo/redo_changes
> don't restore/recover the recog_data, which causes insn_cost takes the
> wrong number of operands (recog_data.n_operands) and wrong operands
> (recog_data.operand[]). Note, rs6000_insn_cost checks if the recog data
> is cached. If so, it doesn't recog the insn again.
>
> void
> insn_info::calculate_cost () const
> {
>   basic_block cfg_bb = BLOCK_FOR_INSN (m_rtl);
>   temporarily_undo_changes (0);
>   m_cost_or_uid = insn_cost (m_rtl, optimize_bb_for_speed_p (cfg_bb));
>   redo_changes (0);
> }
>
> // A simple debug output for calculate_cost
> before undo
> INSN CODE 850
> n_operands 3
>
> after undo
> INSN CODE 685
> n_operands 3
>
> Actually insn pattern 850 has 3 operands and insn pattern 685 has 2
> operands.
>
> IMHO, we should invalidate recog data after each undo/redo or before
> calling the insn cost.

Ah, ok.  If the problem is stale recog_data information, I think we
should instead make recog.cc:swap_change conditionally invalidate it.
I.e. change:

  if (changes[num].object && !MEM_P (changes[num].object))
    std::swap (INSN_CODE (changes[num].object), changes[num].old_code);

to something like:

  if (changes[num].object && !MEM_P (changes[num].object))
    {
      std::swap (INSN_CODE (changes[num].object), changes[num].old_code);
      if (recog_data.insn == changes[num].object)
        recot_data.insn = nullptr;
    }

(untested).  IMO this is better then resetting the INSN_CODE, since the
INSN_CODE should be correct and is relatively expensive to recompute.

Thanks,
Richard
HAO CHEN GUI June 14, 2024, 8:43 a.m. UTC | #5 
在 2024/6/14 16:36, Richard Sandiford 写道:
> Ah, ok.  If the problem is stale recog_data information, I think we
> should instead make recog.cc:swap_change conditionally invalidate it.
> I.e. change:
> 
>   if (changes[num].object && !MEM_P (changes[num].object))
>     std::swap (INSN_CODE (changes[num].object), changes[num].old_code);
> 
> to something like:
> 
>   if (changes[num].object && !MEM_P (changes[num].object))
>     {
>       std::swap (INSN_CODE (changes[num].object), changes[num].old_code);
>       if (recog_data.insn == changes[num].object)
>         recot_data.insn = nullptr;
>     }
> 
> (untested).  IMO this is better then resetting the INSN_CODE, since the
> INSN_CODE should be correct and is relatively expensive to recompute.

It's better. I will modify the patch and test it.

Thanks
Gui Haochen
diff mbox series

Patch

diff --git a/gcc/fwprop.cc b/gcc/fwprop.cc
index de543923b92..975de0eec7f 100644
--- a/gcc/fwprop.cc
+++ b/gcc/fwprop.cc
@@ -471,29 +471,19 @@  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 ()
+      && single_set (use_rtl))
+    {
+      if (!change_is_worthwhile (use_change, false)
+	  || (!prop.likely_profitable_p ()
+	      && !change_is_worthwhile (use_change, true)))
+	{
+	  if (dump_file)
+	    fprintf (dump_file, "change not profitable");
+	  ok = false;
+	}
+    }

   if (!ok)
     {
diff --git a/gcc/rtl-ssa/changes.cc b/gcc/rtl-ssa/changes.cc
index 11639e81bb7..9bad6c2070c 100644
--- a/gcc/rtl-ssa/changes.cc
+++ b/gcc/rtl-ssa/changes.cc
@@ -185,7 +185,18 @@  rtl_ssa::changes_are_worthwhile (array_slice<insn_change *const> changes,
 			      * change->old_cost ());
       if (!change->is_deletion ())
 	{
+	  /* Invalidate recog data as insn_cost may call
+	     extract_insn_cached.  */
+	  INSN_CODE (change->rtl ()) = -1;
 	  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)