[ping #4][patch] Fix PR80929: Realistic PARALLEL cost in seq_cost.

[Georg-Johann Lay]

On 12.07.2017 14:11, Segher Boessenkool wrote:
> Hi,
> 
> On Tue, Jul 11, 2017 at 10:47:27AM +0200, Georg-Johann Lay wrote:
>> This small addition improves costs of PARALLELs in
>> rtlanal.c:seq_cost().  Up to now, these costs are
>> assumed to be 1 which gives gross inexact costs for,
>> e.g. divmod which is represented as PARALLEL.
> 
> insn_rtx_cost returns 0 ("unknown") for such a PARALLEL, so your
> current patch does not change this at all?

Huh?  It returns the costs of 1st SET in a PARALLEL (provided it
has one), no?  Or even costs for come compares.


>> --- rtlanal.c	(revision 248745)
>> +++ rtlanal.c	(working copy)
>> @@ -5300,6 +5300,9 @@ seq_cost (const rtx_insn *seq, bool spee
>>         set = single_set (seq);
>>         if (set)
>>           cost += set_rtx_cost (set, speed);
> 
> So, why does this not use insn_rtx_cost as well?

You'll have to ask the author of that line...

And I didn't intend to change existing behaviour except for a
case where I know that "1" is far off the real costs.

> 
>> +      else if (INSN_P (seq)
>> +	       && PARALLEL == GET_CODE (PATTERN (seq)))
> 
> Yoda conditions have we should not.

hmm, I didn't find something like PARALLEL_P (rtx).
Is comparing rtx_codes deprecated now?

>> +	cost += 1 + insn_rtx_cost (PATTERN (seq), speed);
>>         else
>>           cost++;
>>       }
> 
> This whole thing could be something like
> 
>    if (INSN_P (seq))
>      {
>        int t = insn_rtx_cost (PATTERN (seq), speed);

This will behave differently.  The original set_src_cost calls
rtx_costs with SET and outer_code = INSN, insn_rtx_cost does not.

My intentions was to be conservative and not silently introduce
performance degradations in whatever back-end by changing the
seen RTXes or codes.

Everything that rtx_costs was called for will be the same.
Nothing changes, just some new calls are added.  But neither are
existing calls removed nor are ones changes to up different
arguments.


>        cost += t ? t : COST_N_INSNS (1);
>      }
>    else
>      cost++;
> 
> But set_rtx_cost does *not* return the same answer as insn_rtx_cost.
> 
> (Why do you need a check for INSN_P here?  Why does it increment the

> cost for non-insns?  So many questions).

Again, you'll have to ask the original author for reasoning.

Johann

> 
> 
> Segher
>