HAO CHEN GUI <guih...@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)
