Re: [PATCH 0/5] RFC, WIP: RTL cost improvements

[Richard Sandiford]

Segher Boessenkool  writes:
> On Wed, Aug 09, 2017 at 05:54:40PM +0100, Richard Sandiford wrote:
>> Segher Boessenkool  writes:
>> > We need it, for example, to properly cost the various define_insn_and_split
>> > (for which "type" is only an approximation, and is woefully inadequate
>> > for determining cost).
>> 
>> But define_insn_and_splits could override the cost explicitly if they
>> need to.  That seems neater than testing for them in C.
>
> All 190 of them?  Not counting those that are define_insn+define_split
> (we still have way too many of those).
>
> Neat, indeed, but not altogether practical :-(

Are there really 190 separate cases though?  If not, then it's
possible to have separate attributes that describe various forms of
multi-instruction pattern.  These can then get mapped automatically
to the appropriate "type" and often can also be used to set a
conservatively-correct "length".

Thanks,
Richard

Re: [PATCH 0/5] RFC, WIP: RTL cost improvements

[Segher Boessenkool]

On Wed, Aug 09, 2017 at 05:54:40PM +0100, Richard Sandiford wrote:
> Segher Boessenkool  writes:
> > We need it, for example, to properly cost the various define_insn_and_split
> > (for which "type" is only an approximation, and is woefully inadequate
> > for determining cost).
> 
> But define_insn_and_splits could override the cost explicitly if they
> need to.  That seems neater than testing for them in C.

All 190 of them?  Not counting those that are define_insn+define_split
(we still have way too many of those).

Neat, indeed, but not altogether practical :-(


Segher

Re: [PATCH 0/5] RFC, WIP: RTL cost improvements

[Segher Boessenkool]

On Tue, Aug 08, 2017 at 10:41:05AM -0600, Jeff Law wrote:
> On 08/05/2017 11:15 AM, Segher Boessenkool wrote:
> > For size cost I currently use just "length", but I haven't looked at
> > size cost much at all yet.
> I think that's fine.  "length" is pretty standardized at this point and
> it's the right metric.  For ports that don't bother defining a length
> attribute, punt in some reasonable manner.

I do this is in the target hook, not in generic code.  Commonizing
two lines of code (at the cost of extra complexity for targets that
do *not* want to handle things that way) is not worth it IMO.


Segher

Re: [PATCH 0/5] RFC, WIP: RTL cost improvements

[Richard Sandiford]

Segher Boessenkool  writes:
> On Wed, Aug 09, 2017 at 09:56:31AM -0600, Jeff Law wrote:
>> On 08/08/2017 10:54 AM, Richard Sandiford wrote:
>> > I was thinking we should have separate attributes for size and speed
>> > from the outset.   How about size_cost and speed_cost?  It'd be up
>> > to the target to decide whether to define them as the sums of various
>> > sub-attributes (like it's the target's decision how to break "enabled"
>> > down).
>> But size_cost should be equivalent to the already standardized length
>> attribute.  So I'm struggling to see a need for that.
>
> "length" is (by necessity) pessimistic, cost doesn't have to be.  Not
> that it will make much difference as far as I can see.
>
>> > The advantage of doing it all in attributes is that the generator might
>> > be able to help optimise the process of checking for costs.  No promises
>> > though :-)
>>  :-)
>
> I was thinking I could have "cost" (for rs6000) have a default value
> that looks at "type".  This can then optimise things a little bit.
> But, we probably still need the "function" version as well, for the more
> complex cases.  Simpler targets can do this of course.
>
>> > TBH I think we should start with the attributes as well-defined names
>> > and only add the hook if we find we still need it.
>
> We need it, for example, to properly cost the various define_insn_and_split
> (for which "type" is only an approximation, and is woefully inadequate
> for determining cost).

But define_insn_and_splits could override the cost explicitly if they
need to.  That seems neater than testing for them in C.

>> > I can't really see
>> > what a C function would give over keeping the costs with the instruction
>> > definitions.
>
> There are many insn patterns that can share the cost computation.
>
>> I'd think the C function would mostly be helpful on a cisc target where
>> operands are potentially complex.   But I don't feel strongly enough
>> about it to argue -- I'm happy to go with consensus and fault in
>> adjustments if we need them.
>
> Making this a hook is by far the most flexible.

Maybe, but if so, that would probably also have been true for "enabled"
and "length".  We gained something by not making them hooks.

That said...

> One place where operands are nasty on rs6000 is subregs of float.  Those
> are used all over the place, and they aren't turned into (expensive!)
> insns until LRA.  In the insn patterns they look just like a SI (or DI)
> reg :-(
>
> Another example is unaligned MEMs, which can be very expensive, much
> more expensive than aligned MEMs.

...thanks, I agree these are convincing.

Richard

>
> The common theme is that we care most about cost in the not-so-happy
> cases, and those are often not easy to express in attributes.
>
>
> Segher

Re: [PATCH 0/5] RFC, WIP: RTL cost improvements

[Segher Boessenkool]

On Wed, Aug 09, 2017 at 09:56:31AM -0600, Jeff Law wrote:
> On 08/08/2017 10:54 AM, Richard Sandiford wrote:
> > I was thinking we should have separate attributes for size and speed
> > from the outset.   How about size_cost and speed_cost?  It'd be up
> > to the target to decide whether to define them as the sums of various
> > sub-attributes (like it's the target's decision how to break "enabled"
> > down).
> But size_cost should be equivalent to the already standardized length
> attribute.  So I'm struggling to see a need for that.

"length" is (by necessity) pessimistic, cost doesn't have to be.  Not
that it will make much difference as far as I can see.

> > The advantage of doing it all in attributes is that the generator might
> > be able to help optimise the process of checking for costs.  No promises
> > though :-)
>  :-)

I was thinking I could have "cost" (for rs6000) have a default value
that looks at "type".  This can then optimise things a little bit.
But, we probably still need the "function" version as well, for the more
complex cases.  Simpler targets can do this of course.

> > TBH I think we should start with the attributes as well-defined names
> > and only add the hook if we find we still need it.

We need it, for example, to properly cost the various define_insn_and_split
(for which "type" is only an approximation, and is woefully inadequate
for determining cost).

> > I can't really see
> > what a C function would give over keeping the costs with the instruction
> > definitions.

There are many insn patterns that can share the cost computation.

> I'd think the C function would mostly be helpful on a cisc target where
> operands are potentially complex.   But I don't feel strongly enough
> about it to argue -- I'm happy to go with consensus and fault in
> adjustments if we need them.

Making this a hook is by far the most flexible.

One place where operands are nasty on rs6000 is subregs of float.  Those
are used all over the place, and they aren't turned into (expensive!)
insns until LRA.  In the insn patterns they look just like a SI (or DI)
reg :-(

Another example is unaligned MEMs, which can be very expensive, much
more expensive than aligned MEMs.

The common theme is that we care most about cost in the not-so-happy
cases, and those are often not easy to express in attributes.


Segher

Re: [PATCH 0/5] RFC, WIP: RTL cost improvements

[Richard Sandiford]

Jeff Law  writes:
> On 08/08/2017 10:54 AM, Richard Sandiford wrote:
 For speed cost I primarily use "type", modified by the number of machine
 insns a pattern generates (quite a few are split); and I get the number
 of machine insns just from "length" again, which for rs6000 is easy and
 correct in most cases.  Some other targets may need something else.
>>>

 I also have an attribute "cost" that can be used to override the
 default calculation; that seems useful to standardise on.  I've pondered
 a "cost_adjust" that will be added to the calculated cost instead, but
 it hasn't been useful so far.
>>> Let's go ahead and "reserve" cost and cost_adjust for this purpose.  If
>>> we find that cost_adjust isn't actually necessary, then so be it, it's
>>> not a big deal to me.
>> 
>> I was thinking we should have separate attributes for size and speed
>> from the outset.   How about size_cost and speed_cost?  It'd be up
>> to the target to decide whether to define them as the sums of various
>> sub-attributes (like it's the target's decision how to break "enabled"
>> down).
> But size_cost should be equivalent to the already standardized length
> attribute.  So I'm struggling to see a need for that.

COSTS_N_INSNS lets you add sub-instruction costs, so that you can
slightly prefer faster N-byte instructions to slower N-byte instructions.

> Again, no strong opinions on how to structure the speed cost other than
> to standardize a name.
>
>
>> 
>> The advantage of doing it all in attributes is that the generator might
>> be able to help optimise the process of checking for costs.  No promises
>> though :-)
>  :-)
>
>> 
>> TBH I think we should start with the attributes as well-defined names
>> and only add the hook if we find we still need it.  I can't really see
>> what a C function would give over keeping the costs with the instruction
>> definitions.
> I'd think the C function would mostly be helpful on a cisc target where
> operands are potentially complex.   But I don't feel strongly enough
> about it to argue -- I'm happy to go with consensus and fault in
> adjustments if we need them.

I think even the complex operands can be handled by attributes: you can
have one enum attribute that describes the complexity, perhaps with the
default value calling out to a C function if necessary, and then make
the default value of the cost attribute switch on that enum attribute.

Thanks,
Richard

Re: [PATCH 0/5] RFC, WIP: RTL cost improvements

[Jeff Law]

On 08/08/2017 10:54 AM, Richard Sandiford wrote:
>>> For speed cost I primarily use "type", modified by the number of machine
>>> insns a pattern generates (quite a few are split); and I get the number
>>> of machine insns just from "length" again, which for rs6000 is easy and
>>> correct in most cases.  Some other targets may need something else.
>>
>>>
>>> I also have an attribute "cost" that can be used to override the
>>> default calculation; that seems useful to standardise on.  I've pondered
>>> a "cost_adjust" that will be added to the calculated cost instead, but
>>> it hasn't been useful so far.
>> Let's go ahead and "reserve" cost and cost_adjust for this purpose.  If
>> we find that cost_adjust isn't actually necessary, then so be it, it's
>> not a big deal to me.
> 
> I was thinking we should have separate attributes for size and speed
> from the outset.   How about size_cost and speed_cost?  It'd be up
> to the target to decide whether to define them as the sums of various
> sub-attributes (like it's the target's decision how to break "enabled"
> down).
But size_cost should be equivalent to the already standardized length
attribute.  So I'm struggling to see a need for that.

Again, no strong opinions on how to structure the speed cost other than
to standardize a name.


> 
> The advantage of doing it all in attributes is that the generator might
> be able to help optimise the process of checking for costs.  No promises
> though :-)
 :-)

> 
> TBH I think we should start with the attributes as well-defined names
> and only add the hook if we find we still need it.  I can't really see
> what a C function would give over keeping the costs with the instruction
> definitions.
I'd think the C function would mostly be helpful on a cisc target where
operands are potentially complex.   But I don't feel strongly enough
about it to argue -- I'm happy to go with consensus and fault in
adjustments if we need them.

jeff

Re: [PATCH 0/5] RFC, WIP: RTL cost improvements

[Richard Sandiford]

Jeff Law  writes:
> On 08/05/2017 11:15 AM, Segher Boessenkool wrote:
>> On Sat, Aug 05, 2017 at 11:07:20AM +0100, Richard Sandiford wrote:
>>> Segher Boessenkool  writes:
 This series creates pattern_cost and insn_cost functions that together
 replace the existing insn_rtx_cost function.

 pattern_cost is like the old insn_rtx_cost function; insn_cost takes
 an actual rtx_insn * as input, not just a pattern.

 Also a targetm.insn_cost is added, which targets can use to implement
 a more exact cost more easily.

 The combine patch is pretty gross (but functional), it needs some
 refactoring (to not call recog so often).  The rs6000 patch is very
 much a work in progress.

 How does this look?  Is this the right direction?
>>>
>>> Seems good to me FWIW.  Since this hook is entirely new, would it
>>> be worth standardising on attribute names for size and speed costs,
>>> a bit like "length" and "enabled"?  I think otherwise the target hooks
>>> are going to end up with similar boilerplate.
>> 
>> For size cost I currently use just "length", but I haven't looked at
>> size cost much at all yet.
> I think that's fine.  "length" is pretty standardized at this point and
> it's the right metric.  For ports that don't bother defining a length
> attribute, punt in some reasonable manner.
>
>
>
>> 
>> For speed cost I primarily use "type", modified by the number of machine
>> insns a pattern generates (quite a few are split); and I get the number
>> of machine insns just from "length" again, which for rs6000 is easy and
>> correct in most cases.  Some other targets may need something else.
>
>> 
>> I also have an attribute "cost" that can be used to override the
>> default calculation; that seems useful to standardise on.  I've pondered
>> a "cost_adjust" that will be added to the calculated cost instead, but
>> it hasn't been useful so far.
> Let's go ahead and "reserve" cost and cost_adjust for this purpose.  If
> we find that cost_adjust isn't actually necessary, then so be it, it's
> not a big deal to me.

I was thinking we should have separate attributes for size and speed
from the outset.   How about size_cost and speed_cost?  It'd be up
to the target to decide whether to define them as the sums of various
sub-attributes (like it's the target's decision how to break "enabled"
down).

The advantage of doing it all in attributes is that the generator might
be able to help optimise the process of checking for costs.  No promises
though :-)

TBH I think we should start with the attributes as well-defined names
and only add the hook if we find we still need it.  I can't really see
what a C function would give over keeping the costs with the instruction
definitions.

Thanks,
Richard

Re: [PATCH 0/5] RFC, WIP: RTL cost improvements

[Jeff Law]

On 08/05/2017 11:15 AM, Segher Boessenkool wrote:
> On Sat, Aug 05, 2017 at 11:07:20AM +0100, Richard Sandiford wrote:
>> Segher Boessenkool  writes:
>>> This series creates pattern_cost and insn_cost functions that together
>>> replace the existing insn_rtx_cost function.
>>>
>>> pattern_cost is like the old insn_rtx_cost function; insn_cost takes
>>> an actual rtx_insn * as input, not just a pattern.
>>>
>>> Also a targetm.insn_cost is added, which targets can use to implement
>>> a more exact cost more easily.
>>>
>>> The combine patch is pretty gross (but functional), it needs some
>>> refactoring (to not call recog so often).  The rs6000 patch is very
>>> much a work in progress.
>>>
>>> How does this look?  Is this the right direction?
>>
>> Seems good to me FWIW.  Since this hook is entirely new, would it
>> be worth standardising on attribute names for size and speed costs,
>> a bit like "length" and "enabled"?  I think otherwise the target hooks
>> are going to end up with similar boilerplate.
> 
> For size cost I currently use just "length", but I haven't looked at
> size cost much at all yet.
I think that's fine.  "length" is pretty standardized at this point and
it's the right metric.  For ports that don't bother defining a length
attribute, punt in some reasonable manner.



> 
> For speed cost I primarily use "type", modified by the number of machine
> insns a pattern generates (quite a few are split); and I get the number
> of machine insns just from "length" again, which for rs6000 is easy and
> correct in most cases.  Some other targets may need something else.

> 
> I also have an attribute "cost" that can be used to override the
> default calculation; that seems useful to standardise on.  I've pondered
> a "cost_adjust" that will be added to the calculated cost instead, but
> it hasn't been useful so far.
Let's go ahead and "reserve" cost and cost_adjust for this purpose.  If
we find that cost_adjust isn't actually necessary, then so be it, it's
not a big deal to me.

Jeff

Re: [PATCH 0/5] RFC, WIP: RTL cost improvements

[Segher Boessenkool]

On Sat, Aug 05, 2017 at 11:07:20AM +0100, Richard Sandiford wrote:
> Segher Boessenkool  writes:
> > This series creates pattern_cost and insn_cost functions that together
> > replace the existing insn_rtx_cost function.
> >
> > pattern_cost is like the old insn_rtx_cost function; insn_cost takes
> > an actual rtx_insn * as input, not just a pattern.
> >
> > Also a targetm.insn_cost is added, which targets can use to implement
> > a more exact cost more easily.
> >
> > The combine patch is pretty gross (but functional), it needs some
> > refactoring (to not call recog so often).  The rs6000 patch is very
> > much a work in progress.
> >
> > How does this look?  Is this the right direction?
> 
> Seems good to me FWIW.  Since this hook is entirely new, would it
> be worth standardising on attribute names for size and speed costs,
> a bit like "length" and "enabled"?  I think otherwise the target hooks
> are going to end up with similar boilerplate.

For size cost I currently use just "length", but I haven't looked at
size cost much at all yet.

For speed cost I primarily use "type", modified by the number of machine
insns a pattern generates (quite a few are split); and I get the number
of machine insns just from "length" again, which for rs6000 is easy and
correct in most cases.  Some other targets may need something else.

I also have an attribute "cost" that can be used to override the
default calculation; that seems useful to standardise on.  I've pondered
a "cost_adjust" that will be added to the calculated cost instead, but
it hasn't been useful so far.

There are two types of insns that happen in the insn stream but which
aren't recognised: asm, and no-op moves.  I cost both as cost 0 currently.

"length" and "enabled" are special, they are used by non-target code.
So far I don't think it is useful to handle "cost" specially, but it
is such a nice short name that I expect many targets will want it,
whether it is handled generically or not :-)


Segher

Re: [PATCH 0/5] RFC, WIP: RTL cost improvements

[Richard Sandiford]

Segher Boessenkool  writes:
> This series creates pattern_cost and insn_cost functions that together
> replace the existing insn_rtx_cost function.
>
> pattern_cost is like the old insn_rtx_cost function; insn_cost takes
> an actual rtx_insn * as input, not just a pattern.
>
> Also a targetm.insn_cost is added, which targets can use to implement
> a more exact cost more easily.
>
> The combine patch is pretty gross (but functional), it needs some
> refactoring (to not call recog so often).  The rs6000 patch is very
> much a work in progress.
>
> How does this look?  Is this the right direction?

Seems good to me FWIW.  Since this hook is entirely new, would it
be worth standardising on attribute names for size and speed costs,
a bit like "length" and "enabled"?  I think otherwise the target hooks
are going to end up with similar boilerplate.

Thanks,
Richard

Re: [PATCH 0/5] RFC, WIP: RTL cost improvements

[Richard Biener]

On Tue, Aug 1, 2017 at 1:24 AM, Segher Boessenkool
 wrote:
> This series creates pattern_cost and insn_cost functions that together
> replace the existing insn_rtx_cost function.
>
> pattern_cost is like the old insn_rtx_cost function; insn_cost takes
> an actual rtx_insn * as input, not just a pattern.
>
> Also a targetm.insn_cost is added, which targets can use to implement
> a more exact cost more easily.
>
> The combine patch is pretty gross (but functional), it needs some
> refactoring (to not call recog so often).  The rs6000 patch is very
> much a work in progress.
>
> How does this look?  Is this the right direction?

I think it is the right direction.  I'll leave the details to somebody more
familiar with RTL.

Richard.

>
> Segher
>
>
> Segher Boessenkool (5):
>   Rename existing insn_cost to insn_sched_cost
>   Replace insn_rtx_cost with insn_cost and pattern_cost
>   combine: Use insn_cost instead of pattern_cost everywhere
>   Add targetm.insn_cost hook
>   rs6000: Implement insn_cost hook
>
>  gcc/cfgrtl.c   |  7 +++
>  gcc/combine.c  | 40 +---
>  gcc/config/rs6000/rs6000.c | 14 ++
>  gcc/doc/tm.texi| 12 
>  gcc/doc/tm.texi.in |  2 ++
>  gcc/dse.c  |  2 +-
>  gcc/haifa-sched.c  | 14 +++---
>  gcc/ifcvt.c| 12 ++--
>  gcc/rtl.h  |  3 ++-
>  gcc/rtlanal.c  | 16 ++--
>  gcc/sched-int.h|  2 +-
>  gcc/sched-rgn.c|  4 ++--
>  gcc/sel-sched-ir.c |  2 +-
>  gcc/target.def | 14 ++
>  14 files changed, 108 insertions(+), 36 deletions(-)
>
> --
> 1.9.3
>

[PATCH 0/5] RFC, WIP: RTL cost improvements

[Segher Boessenkool]

This series creates pattern_cost and insn_cost functions that together
replace the existing insn_rtx_cost function.

pattern_cost is like the old insn_rtx_cost function; insn_cost takes
an actual rtx_insn * as input, not just a pattern.

Also a targetm.insn_cost is added, which targets can use to implement
a more exact cost more easily.

The combine patch is pretty gross (but functional), it needs some
refactoring (to not call recog so often).  The rs6000 patch is very
much a work in progress.

How does this look?  Is this the right direction?


Segher


Segher Boessenkool (5):
  Rename existing insn_cost to insn_sched_cost
  Replace insn_rtx_cost with insn_cost and pattern_cost
  combine: Use insn_cost instead of pattern_cost everywhere
  Add targetm.insn_cost hook
  rs6000: Implement insn_cost hook

 gcc/cfgrtl.c   |  7 +++
 gcc/combine.c  | 40 +---
 gcc/config/rs6000/rs6000.c | 14 ++
 gcc/doc/tm.texi| 12 
 gcc/doc/tm.texi.in |  2 ++
 gcc/dse.c  |  2 +-
 gcc/haifa-sched.c  | 14 +++---
 gcc/ifcvt.c| 12 ++--
 gcc/rtl.h  |  3 ++-
 gcc/rtlanal.c  | 16 ++--
 gcc/sched-int.h|  2 +-
 gcc/sched-rgn.c|  4 ++--
 gcc/sel-sched-ir.c |  2 +-
 gcc/target.def | 14 ++
 14 files changed, 108 insertions(+), 36 deletions(-)

-- 
1.9.3