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. Remove !single_set at checking likely_profitable_p. Add is_debug_insn
> here, so that debug insn still need to be profitable.
> 2. Remove single_set check for cost comparison. Add !is_debug_insn here,
> so that debug insn doesn't need to check the insn cost.
>
> Previous version
> https://gcc.gnu.org/pipermail/gcc-patches/2024-June/654964.html
>
> The patch causes a regression case 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?
>
> Thanks
> Gui Haochen
>
> 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. Remove single_set check
> and add is_debug_insn check.
> * 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..60e3ea31edc 100644
> --- a/gcc/fwprop.cc
> +++ b/gcc/fwprop.cc
> @@ -453,7 +453,7 @@ try_fwprop_subst_pattern (obstack_watermark &attempt,
> insn_change &use_change,
> && (prop.changed_mem_p ()
> || contains_mem_rtx_p (src)
> || use_insn->is_asm ()
> - || !single_set (use_rtl)))
> + || use_insn->is_debug_insn ()))
> {
> if (dump_file && (dump_flags & TDF_DETAILS))
> fprintf (dump_file, "cannot propagate from insn %d into"
Hmm. It's hard to tell whether the author was deliberately excluding
debug insns here, or whether doing that was just a side-effect of
disallowing multiple sets.
I'm not sure "profitability" means much in the case of debug insns.
There again, I suppose having no limit on the complexity of debug
insns would risk quadraticness and (even without that) would tend
to increase the size of the on-disk debug info.
So I agree that the change is correct and that we should continue
to disallow debug insns here.
> @@ -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 ()
> + && !use_insn->is_debug_insn ())
Very minor nit, but now that the condition doesn't fit on a single line,
each "&&" should be on its own line:
if (ok
&& !prop.changed_mem_p ()
&& !use_insn->is_asm ()
&& !use_insn->is_debug_insn ())
OK with that formatting change, 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<insn_change
> *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)
