[Ping,#3] PR80929: Realistic PARALLEL cost in seq_cost.

On 28.06.2017 22:18, Wilco Dijkstra wrote:
> Georg-Johann Lay wrote:
> 
> @@ -5300,6 +5300,9 @@ seq_cost (const rtx_insn *seq, bool spee
>         set = single_set (seq);
>         if (set)
>           cost += set_rtx_cost (set, speed);
> +      else if (INSN_P (seq)
> +	       && PARALLEL == GET_CODE (PATTERN (seq)))
> +	cost += insn_rtx_cost (PATTERN (seq), speed);
>         else
>           cost++;
> 
> insn_rtx_cost may return zero if it can't find something useful in the parallel,
> which means it may return a lower cost and even zero. Not sure whether this
> is important, but in eg. combine a cost of zero means infinite and so could have
> unintended consequences. So incrementing cost with a non-zero value
> if insn_rtx_cost == 0 would seem safer.

Updated patch below, it just adds 1 (which is 1/4 of CONST_N_INSNS) to
avoid zero.


> Also why does the else do cost++ and not cost += COSTS_N_INSNS (1)?
> 
> Wilco

Dunno, I didn't change this.  Maybe it's also just to escape 0.

Johann

gcc/
     PR middle-end/80929
     * rtlanal.c (seq_cost) [PARALLEL]: Get cost from insn_rtx_cost
     instead of assuming cost of 1.

On 06/29/2017 02:51 AM, Georg-Johann Lay wrote:
> On 28.06.2017 22:18, Wilco Dijkstra wrote:
>> Georg-Johann Lay wrote:
>>
>> @@ -5300,6 +5300,9 @@ seq_cost (const rtx_insn *seq, bool spee
>>         set = single_set (seq);
>>         if (set)
>>           cost += set_rtx_cost (set, speed);
>> +      else if (INSN_P (seq)
>> +           && PARALLEL == GET_CODE (PATTERN (seq)))
>> +    cost += insn_rtx_cost (PATTERN (seq), speed);
>>         else
>>           cost++;
>>
>> insn_rtx_cost may return zero if it can't find something useful in the
>> parallel,
>> which means it may return a lower cost and even zero. Not sure whether
>> this
>> is important, but in eg. combine a cost of zero means infinite and so
>> could have
>> unintended consequences. So incrementing cost with a non-zero value
>> if insn_rtx_cost == 0 would seem safer.
> 
> Updated patch below, it just adds 1 (which is 1/4 of CONST_N_INSNS) to
> avoid zero.
> 
> 
>> Also why does the else do cost++ and not cost += COSTS_N_INSNS (1)?
>>
>> Wilco
> 
> Dunno, I didn't change this.  Maybe it's also just to escape 0.
> 
> Johann
> 
> gcc/
>     PR middle-end/80929
>     * rtlanal.c (seq_cost) [PARALLEL]: Get cost from insn_rtx_cost
>     instead of assuming cost of 1.
So this has probably been hashed to death, but just a couple thoughts.

I think realistically one has to look at the entirety of the PARALLEL to
get any reasonable costing.

Summing the individual components is wrong.  Taking the cost of any
individual component is wrong.  You might be able to make a case for the
maximum cost of the individual components, but even that is almost
certain to be wrong in some cases.


Presumably this is part of what Segher is trying to address with his
costing changes.

jeff

On Wed, Aug 02, 2017 at 12:17:44PM -0600, Jeff Law wrote:
> So this has probably been hashed to death, but just a couple thoughts.
> 
> I think realistically one has to look at the entirety of the PARALLEL to
> get any reasonable costing.
> 
> Summing the individual components is wrong.  Taking the cost of any
> individual component is wrong.  You might be able to make a case for the
> maximum cost of the individual components, but even that is almost
> certain to be wrong in some cases.

I have tried all of these over the years.  Nothing is good for all
targets and all insns for a target, alas.

> Presumably this is part of what Segher is trying to address with his
> costing changes.

Sure, seq_cost is trivial to express in terms of insn_cost.  I'll add
a patch for it.

(set_rtx_cost, which is what seq_cost currently uses, is most often
used for RTL that isn't (yet) in an insn.  We'll have to look per case
how to best handle that.  Ideally rtx_cost will go away or only continue
in a much slimmed down form, for targets that implement insn_cost).


Segher