Hi Richard,
Thanks for your comments.
在 2024/6/12 15:51, Richard Sandiford 写道:
> 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;
> }
> }
I will modify the code. Thanks.
> 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.
Yes, the recog sets the recog data for the new rtl. But
changes_are_worthwhile calls change->old_cost and finally calls
calculate_cost to get the insn cost for old rtl. The undo/redo_changes
don't restore/recover the recog_data, which causes insn_cost takes the
wrong number of operands (recog_data.n_operands) and wrong operands
(recog_data.operand[]). Note, rs6000_insn_cost checks if the recog data
is cached. If so, it doesn't recog the insn again.
void
insn_info::calculate_cost () const
{
basic_block cfg_bb = BLOCK_FOR_INSN (m_rtl);
temporarily_undo_changes (0);
m_cost_or_uid = insn_cost (m_rtl, optimize_bb_for_speed_p (cfg_bb));
redo_changes (0);
}
// A simple debug output for calculate_cost
before undo
INSN CODE 850
n_operands 3
after undo
INSN CODE 685
n_operands 3
Actually insn pattern 850 has 3 operands and insn pattern 685 has 2
operands.
IMHO, we should invalidate recog data after each undo/redo or before
calling the insn cost.
Looking forward to your advice.
Thanks
Gui Haochen
