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

[PATCH-1] 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 kind of
destination operand (memory or pseudo) decides the cost and rtx cost
can't reflect it.

  The test case is added in the second target specific patch.

  Bootstrapped and tested on x86 and powerpc64-linux BE and LE with no
regressions. Is it OK for next stage 1?

Thanks
Gui Haochen


ChangeLog
fwprop: Replace rtx_cost with insn_cost in try_fwprop_subst_pattern

gcc/
	PR target/113325
	* fwprop.cc (try_fwprop_subst_pattern): Replace rtx_cost with
	insn_cost.


patch.diff
diff --git a/gcc/fwprop.cc b/gcc/fwprop.cc
index 0707a234726..b05b2538edc 100644
--- a/gcc/fwprop.cc
+++ b/gcc/fwprop.cc
@@ -467,20 +467,17 @@ 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))
+    if (single_set (use_rtl))
       {
 	bool speed = optimize_bb_for_speed_p (BLOCK_FOR_INSN (use_rtl));
+	auto new_cost = insn_cost (use_rtl, speed);
 	temporarily_undo_changes (0);
-	auto old_cost = set_src_cost (SET_SRC (use_set),
-				      GET_MODE (SET_DEST (use_set)), speed);
+	/* Invalidate recog data.  */
+	INSN_CODE (use_rtl) = -1;
+	auto old_cost = insn_cost (use_rtl, 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)
 	  {
 	    if (dump_file)

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

[Jeff Law]

On 1/25/24 6:16 PM, HAO CHEN GUI wrote:
> 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 kind of
> destination operand (memory or pseudo) decides the cost and rtx cost
> can't reflect it.
> 
>    The test case is added in the second target specific patch.
> 
>    Bootstrapped and tested on x86 and powerpc64-linux BE and LE with no
> regressions. Is it OK for next stage 1?
> 
> Thanks
> Gui Haochen
> 
> 
> ChangeLog
> fwprop: Replace rtx_cost with insn_cost in try_fwprop_subst_pattern
> 
> gcc/
> 	PR target/113325
> 	* fwprop.cc (try_fwprop_subst_pattern): Replace rtx_cost with
> 	insn_cost.
Testcase?  I don't care of it's ppc specific.

I think we generally want to move from rtx_cost to insn_cost, so I think 
the change itself is fine.  We just want to make sure a test covers the 
change in some manner.

Also note this a change to generic code and could likely trigger 
failures on various targets that have assembler scanning tests.  So once 
you've got a testcase and the full patch is ack'd we'll need to watch 
closely for regressions reported on other targets.


So ACK'd once you add a testcase.

Jeff

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

[HAO CHEN GUI]

Hi Jeff,

在 2024/6/4 22:14, Jeff Law 写道:
> 
> 
> On 1/25/24 6:16 PM, HAO CHEN GUI wrote:
>> 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 kind of
>> destination operand (memory or pseudo) decides the cost and rtx cost
>> can't reflect it.
>>
>>    The test case is added in the second target specific patch.
>>
>>    Bootstrapped and tested on x86 and powerpc64-linux BE and LE with no
>> regressions. Is it OK for next stage 1?
>>
>> Thanks
>> Gui Haochen
>>
>>
>> ChangeLog
>> fwprop: Replace rtx_cost with insn_cost in try_fwprop_subst_pattern
>>
>> gcc/
>>     PR target/113325
>>     * fwprop.cc (try_fwprop_subst_pattern): Replace rtx_cost with
>>     insn_cost.
> Testcase?  I don't care of it's ppc specific.
> 
> I think we generally want to move from rtx_cost to insn_cost, so I think the change itself is fine.  We just want to make sure a test covers the change in some manner.
> 
> Also note this a change to generic code and could likely trigger failures on various targets that have assembler scanning tests.  So once you've got a testcase and the full patch is ack'd we'll need to watch closely for regressions reported on other targets.
> 
> 
> So ACK'd once you add a testcase.
> 
> Jeff
Thanks for your comments.

The test case is in this rs6000 patch. The patch is still under review.
https://gcc.gnu.org/pipermail/gcc-patches/2024-January/643995.html

I have sent the second version of the patch. The main change is to detect the
zero cost returned by insn_cost as it means the cost is unknown.
https://gcc.gnu.org/pipermail/gcc-patches/2024-May/651233.html

I have already tested the patch on other targets. I have found some regression
on x86 due to the wrong cost conversion from set_src_cost to pattern_cost. I
have sent another patch for this issue. Reviewers have different thoughts on
it. It's pending now.
https://gcc.gnu.org/pipermail/gcc-patches/2024-May/651363.html