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)
