Re: [PATCH-1v4] fwprop: Replace rtx_cost with insn_cost in try_fwprop_subst_pattern [PR113325]
HAO CHEN GUI writes:
> Hi Richard,
>
> 在 2024/6/17 17:04, Richard Sandiford 写道:
>> I don't think we should keep the single_set condition after this change.
>> insn_cost can handle all instructions.
>
> Just tested with removing single_set condition. It causes some regressions.
> If the new_rtl is a debug insn, it still can do the replacement as it can
> be recog and it's not a single_set rtl. After removing single_set condition,
> the "ok" (recog) will be reset to false in the "if" block as the cost of
> debug insn is unknown. So the replacement won't be done.
>
> 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;
> }
> }
>
> if (!ok)
> {
> /* The pattern didn't match, but if all uses of SRC folded to
> constants, we can add a REG_EQUAL note for the result, if there
> isn't one already. */
> if (!prop.folded_to_constants_p ())
We shouldn't be trying to cost changes to debug insns, so I think
the extra condition should be !use_insn->is_debug_insn ().
Thanks,
Richard
Re: [PATCH-1v4] fwprop: Replace rtx_cost with insn_cost in try_fwprop_subst_pattern [PR113325]
Hi Richard,
在 2024/6/17 17:04, Richard Sandiford 写道:
> I don't think we should keep the single_set condition after this change.
> insn_cost can handle all instructions.
Just tested with removing single_set condition. It causes some regressions.
If the new_rtl is a debug insn, it still can do the replacement as it can
be recog and it's not a single_set rtl. After removing single_set condition,
the "ok" (recog) will be reset to false in the "if" block as the cost of
debug insn is unknown. So the replacement won't be done.
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;
}
}
if (!ok)
{
/* The pattern didn't match, but if all uses of SRC folded to
constants, we can add a REG_EQUAL note for the result, if there
isn't one already. */
if (!prop.folded_to_constants_p ())
Looking forwarding to your advice.
Thanks
Gui Haochen
[PATCH-1v4] fwprop: Replace rtx_cost with insn_cost in try_fwprop_subst_pattern [PR113325]
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. Invalidate recog_data when the cached INSN is swapped out.
2. Pass strict_p according to prop.likely_profitable_p () to
change_is_worthwhile.
Previous version
https://gcc.gnu.org/pipermail/gcc-patches/2024-June/654276.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.
* 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..4a9f68b66b1 100644
--- a/gcc/fwprop.cc
+++ b/gcc/fwprop.cc
@@ -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 ()
+ && single_set (use_rtl))
+{
+ 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 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-1v4] fwprop: Replace rtx_cost with insn_cost in try_fwprop_subst_pattern [PR113325]
HAO CHEN GUI 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. Invalidate recog_data when the cached INSN is swapped out.
> 2. Pass strict_p according to prop.likely_profitable_p () to
> change_is_worthwhile.
>
> Previous version
> https://gcc.gnu.org/pipermail/gcc-patches/2024-June/654276.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.
> * 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..4a9f68b66b1 100644
> --- a/gcc/fwprop.cc
> +++ b/gcc/fwprop.cc
> @@ -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 ()
> + && single_set (use_rtl))
I don't think we should keep the single_set condition after this change.
insn_cost can handle all instructions.
OK for trunk with that removed.
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 *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)
