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

[Jeff Law <jeffreyalaw@gmail.com>]

On 6/5/24 3:08 AM, Richard Sandiford wrote:
> HAO CHEN GUI <guihaoc@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 target specific patch.
>> https://gcc.gnu.org/pipermail/gcc-patches/2024-January/643995.html
>>
>>    Compared to previous version, the main change is not to do
>> substitution if either new or old insn cost is zero. The zero means
>> the cost is unknown.
>>
>>   Previous version
>> https://gcc.gnu.org/pipermail/gcc-patches/2024-January/643994.html
>>
>>    Bootstrapped and tested on x86 and powerpc64-linux BE and LE with no
>> regressions. Is it OK for the trunk?
>>
>> ChangeLog
>> fwprop: Replace set_src_cost with insn_cost in try_fwprop_subst_pattern
>>
>> gcc/
>> 	* fwprop.cc (try_fwprop_subst_pattern): Replace set_src_cost with
>> 	insn_cost.
> 
> Thanks for doing this.  It's definitely the right direction, but:
> 
>> patch.diff
>> diff --git a/gcc/fwprop.cc b/gcc/fwprop.cc
>> index cb6fd6700ca..184a22678b7 100644
>> --- a/gcc/fwprop.cc
>> +++ b/gcc/fwprop.cc
>> @@ -470,21 +470,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.  */
> 
> ...as hinted at in the comment, rtl-ssa already has a routine for
> insn_cost-based calculations.  It has two (supposed) advantages:
> it caches the old costs, and it takes execution frequency into
> account when optimising for speed.
> 
> The comment is out of date though.  The name of the routine is
> changes_are_worthwhile rather than change_is_worthwhile.  Could you
> try using that instead?
Funny, I went wandering around looking for that function to see how it 
was implemented and how it might compare to what was being proposed.

Of course I never found it, even after rewinding to various old git 
hashes that looked promising.

So, yea, definitely would prefer re-using changes_are_worthwhile if it 
works reasonably well for the issue at hand.

jeff