Missing CC to Jeff Law. Sorry.
在 2024/6/12 10:41, 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 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; > + } > + } > > if (!ok) > { > 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; > 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)
