Re: [PATCH-1v3] fwprop: Replace rtx_cost with insn_cost in try_fwprop_subst_pattern [PR113325]

[Richard Sandiford]

HAO CHEN GUI <guih...@linux.ibm.com> writes:
>> 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.

Ah, ok.  If the problem is stale recog_data information, I think we
should instead make recog.cc:swap_change conditionally invalidate it.
I.e. change:

  if (changes[num].object && !MEM_P (changes[num].object))
    std::swap (INSN_CODE (changes[num].object), changes[num].old_code);

to something like:

  if (changes[num].object && !MEM_P (changes[num].object))
    {
      std::swap (INSN_CODE (changes[num].object), changes[num].old_code);
      if (recog_data.insn == changes[num].object)
        recot_data.insn = nullptr;
    }

(untested).  IMO this is better then resetting the INSN_CODE, since the
INSN_CODE should be correct and is relatively expensive to recompute.

Thanks,
Richard