* [PATCH-1v2] fwprop: Replace rtx_cost with insn_cost in try_fwprop_subst_pattern [PR113325]
@ 2024-05-10 5:43 HAO CHEN GUI
2024-06-05 9:08 ` Richard Sandiford
0 siblings, 1 reply; 3+ messages in thread
From: HAO CHEN GUI @ 2024-05-10 5:43 UTC (permalink / raw)
To: gcc-patches
Cc: Segher Boessenkool, David, Kewen.Lin, Peter Bergner,
Richard Sandiford, Jeff Law
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 target specific patch.
https://gcc.gnu.org/pipermail/gcc-patches/2024-January/643995.html
Compared to previous version, the main change is not to do
substitution if either new or old insn cost is zero. The zero means
the cost is unknown.
Previous version
https://gcc.gnu.org/pipermail/gcc-patches/2024-January/643994.html
Bootstrapped and tested on x86 and powerpc64-linux BE and LE with no
regressions. Is it OK for the trunk?
ChangeLog
fwprop: Replace set_src_cost with insn_cost in try_fwprop_subst_pattern
gcc/
* fwprop.cc (try_fwprop_subst_pattern): Replace set_src_cost with
insn_cost.
patch.diff
diff --git a/gcc/fwprop.cc b/gcc/fwprop.cc
index cb6fd6700ca..184a22678b7 100644
--- a/gcc/fwprop.cc
+++ b/gcc/fwprop.cc
@@ -470,21 +470,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))
+ 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);
+ /* Invalide 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 (new_cost == 0 || old_cost == 0
+ || new_cost > old_cost
|| (new_cost == old_cost && !prop.likely_profitable_p ()))
{
if (dump_file)
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH-1v2] fwprop: Replace rtx_cost with insn_cost in try_fwprop_subst_pattern [PR113325]
2024-05-10 5:43 [PATCH-1v2] fwprop: Replace rtx_cost with insn_cost in try_fwprop_subst_pattern [PR113325] HAO CHEN GUI
@ 2024-06-05 9:08 ` Richard Sandiford
2024-06-05 13:27 ` Jeff Law
0 siblings, 1 reply; 3+ messages in thread
From: Richard Sandiford @ 2024-06-05 9:08 UTC (permalink / raw)
To: HAO CHEN GUI
Cc: gcc-patches, Segher Boessenkool, David, Kewen.Lin, Peter Bergner,
Jeff Law
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 target specific patch.
> https://gcc.gnu.org/pipermail/gcc-patches/2024-January/643995.html
>
> Compared to previous version, the main change is not to do
> substitution if either new or old insn cost is zero. The zero means
> the cost is unknown.
>
> Previous version
> https://gcc.gnu.org/pipermail/gcc-patches/2024-January/643994.html
>
> Bootstrapped and tested on x86 and powerpc64-linux BE and LE with no
> regressions. Is it OK for the trunk?
>
> ChangeLog
> fwprop: Replace set_src_cost with insn_cost in try_fwprop_subst_pattern
>
> gcc/
> * fwprop.cc (try_fwprop_subst_pattern): Replace set_src_cost with
> insn_cost.
Thanks for doing this. It's definitely the right direction, but:
> patch.diff
> diff --git a/gcc/fwprop.cc b/gcc/fwprop.cc
> index cb6fd6700ca..184a22678b7 100644
> --- a/gcc/fwprop.cc
> +++ b/gcc/fwprop.cc
> @@ -470,21 +470,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. */
...as hinted at in the comment, rtl-ssa already has a routine for
insn_cost-based calculations. It has two (supposed) advantages:
it caches the old costs, and it takes execution frequency into
account when optimising for speed.
The comment is out of date though. The name of the routine is
changes_are_worthwhile rather than change_is_worthwhile. Could you
try using that instead?
Richard
> 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);
> + /* Invalide 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 (new_cost == 0 || old_cost == 0
> + || new_cost > old_cost
> || (new_cost == old_cost && !prop.likely_profitable_p ()))
> {
> if (dump_file)
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH-1v2] fwprop: Replace rtx_cost with insn_cost in try_fwprop_subst_pattern [PR113325]
2024-06-05 9:08 ` Richard Sandiford
@ 2024-06-05 13:27 ` Jeff Law
0 siblings, 0 replies; 3+ messages in thread
From: Jeff Law @ 2024-06-05 13:27 UTC (permalink / raw)
To: HAO CHEN GUI, gcc-patches, Segher Boessenkool, David, Kewen.Lin,
Peter Bergner, richard.sandiford
On 6/5/24 3:08 AM, Richard Sandiford wrote:
> 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 target specific patch.
>> https://gcc.gnu.org/pipermail/gcc-patches/2024-January/643995.html
>>
>> Compared to previous version, the main change is not to do
>> substitution if either new or old insn cost is zero. The zero means
>> the cost is unknown.
>>
>> Previous version
>> https://gcc.gnu.org/pipermail/gcc-patches/2024-January/643994.html
>>
>> Bootstrapped and tested on x86 and powerpc64-linux BE and LE with no
>> regressions. Is it OK for the trunk?
>>
>> ChangeLog
>> fwprop: Replace set_src_cost with insn_cost in try_fwprop_subst_pattern
>>
>> gcc/
>> * fwprop.cc (try_fwprop_subst_pattern): Replace set_src_cost with
>> insn_cost.
>
> Thanks for doing this. It's definitely the right direction, but:
>
>> patch.diff
>> diff --git a/gcc/fwprop.cc b/gcc/fwprop.cc
>> index cb6fd6700ca..184a22678b7 100644
>> --- a/gcc/fwprop.cc
>> +++ b/gcc/fwprop.cc
>> @@ -470,21 +470,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. */
>
> ...as hinted at in the comment, rtl-ssa already has a routine for
> insn_cost-based calculations. It has two (supposed) advantages:
> it caches the old costs, and it takes execution frequency into
> account when optimising for speed.
>
> The comment is out of date though. The name of the routine is
> changes_are_worthwhile rather than change_is_worthwhile. Could you
> try using that instead?
Funny, I went wandering around looking for that function to see how it
was implemented and how it might compare to what was being proposed.
Of course I never found it, even after rewinding to various old git
hashes that looked promising.
So, yea, definitely would prefer re-using changes_are_worthwhile if it
works reasonably well for the issue at hand.
jeff
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-06-05 13:27 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-05-10 5:43 [PATCH-1v2] fwprop: Replace rtx_cost with insn_cost in try_fwprop_subst_pattern [PR113325] HAO CHEN GUI
2024-06-05 9:08 ` Richard Sandiford
2024-06-05 13:27 ` Jeff Law
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).