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

Ping #4

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.

The patch just forwards cost computation to insn_rtx_cost
which uses the cost of the 1st SET (if any) and otherwise
assign costs of 1 insn.

Bootstrapped & regtested on x86_64.

Moreover, it fixed the division by constant on avr where
the problem popped up since PR79665.

Ok to install?

Johann

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

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?

> --- 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?

> +      else if (INSN_P (seq)
> +	       && PARALLEL == GET_CODE (PATTERN (seq)))

Yoda conditions have we should not.

> +	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);
      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).


Segher

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
>

On Wed, Jul 12, 2017 at 03:30:00PM +0200, Georg-Johann Lay wrote:
> On 12.07.2017 14:11, Segher Boessenkool wrote:
> >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.

No, it returns 0 if there is more than one normal SET (or more than
one compare).

> >>+      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?

I meant it should be written

  else if (INSN_P (seq) && GET_CODE (PATTERN (seq)) == PARALLEL)

i.e. constant on the right.

> >This whole thing could be something like
> >
> >   if (INSN_P (seq))
> >     {
> >       int t = insn_rtx_cost (PATTERN (seq), speed);
> 
> This will behave differently.

Yes, I know, I even said so.

> >(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.

Since you want to change the code, to make it better, I was hoping
you would dig in a bit.  To make it better, not just different :-/


Segher

On 12.07.2017 21:36, Segher Boessenkool wrote:
> On Wed, Jul 12, 2017 at 03:30:00PM +0200, Georg-Johann Lay wrote:
>> On 12.07.2017 14:11, Segher Boessenkool wrote:
>>> 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?

Ah I see now.

So this is unfixable...

Any change to seq_cost that would address the issue I had in mind
(completely wrong costs for divmod that are represented as PARALLEL
with 2 SETs) will come up with different handling than the "logic"
of insn_rtx_costs.

So the only way to avoid that "logic" is to pass the whole story
to the back-end.

And in order not to break any existing assumptions this can only
be achieved by a new hook that graceful degrades to the current
behaviour and reasoning when that new hook says "dunno".

I already started an RFC here:

https://gcc.gnu.org/ml/gcc/2017-07/msg00080.html


Johann