* [ping #4][patch] Fix PR80929: Realistic PARALLEL cost in seq_cost.
@ 2017-07-11 8:47 Georg-Johann Lay
2017-07-12 12:11 ` Segher Boessenkool
0 siblings, 1 reply; 5+ messages in thread
From: Georg-Johann Lay @ 2017-07-11 8:47 UTC (permalink / raw)
To: gcc-patches
[-- Attachment #1: Type: text/plain, Size: 629 bytes --]
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.
[-- Attachment #2: pr80929.diff --]
[-- Type: text/x-patch, Size: 467 bytes --]
Index: rtlanal.c
===================================================================
--- 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);
+ else if (INSN_P (seq)
+ && PARALLEL == GET_CODE (PATTERN (seq)))
+ cost += 1 + insn_rtx_cost (PATTERN (seq), speed);
else
cost++;
}
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [ping #4][patch] Fix PR80929: Realistic PARALLEL cost in seq_cost.
2017-07-11 8:47 [ping #4][patch] Fix PR80929: Realistic PARALLEL cost in seq_cost Georg-Johann Lay
@ 2017-07-12 12:11 ` Segher Boessenkool
2017-07-12 13:30 ` Georg-Johann Lay
0 siblings, 1 reply; 5+ messages in thread
From: Segher Boessenkool @ 2017-07-12 12:11 UTC (permalink / raw)
To: Georg-Johann Lay; +Cc: gcc-patches
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [ping #4][patch] Fix PR80929: Realistic PARALLEL cost in seq_cost.
2017-07-12 12:11 ` Segher Boessenkool
@ 2017-07-12 13:30 ` Georg-Johann Lay
2017-07-12 19:36 ` Segher Boessenkool
0 siblings, 1 reply; 5+ messages in thread
From: Georg-Johann Lay @ 2017-07-12 13:30 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: gcc-patches
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [ping #4][patch] Fix PR80929: Realistic PARALLEL cost in seq_cost.
2017-07-12 13:30 ` Georg-Johann Lay
@ 2017-07-12 19:36 ` Segher Boessenkool
2017-07-13 11:04 ` Georg-Johann Lay
0 siblings, 1 reply; 5+ messages in thread
From: Segher Boessenkool @ 2017-07-12 19:36 UTC (permalink / raw)
To: Georg-Johann Lay; +Cc: gcc-patches
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [ping #4][patch] Fix PR80929: Realistic PARALLEL cost in seq_cost.
2017-07-12 19:36 ` Segher Boessenkool
@ 2017-07-13 11:04 ` Georg-Johann Lay
0 siblings, 0 replies; 5+ messages in thread
From: Georg-Johann Lay @ 2017-07-13 11:04 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: gcc-patches
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-07-13 11:04 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-11 8:47 [ping #4][patch] Fix PR80929: Realistic PARALLEL cost in seq_cost Georg-Johann Lay
2017-07-12 12:11 ` Segher Boessenkool
2017-07-12 13:30 ` Georg-Johann Lay
2017-07-12 19:36 ` Segher Boessenkool
2017-07-13 11:04 ` Georg-Johann Lay
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).