[patch] RFC: Hook for insn costs?

[Georg-Johann Lay]

On 16.07.2017 00:51, Segher Boessenkool wrote:
> Hi!
> 
> On Wed, Jul 12, 2017 at 03:15:09PM +0200, Georg-Johann Lay wrote:
>> the current cost computations in rtlanal.c and maybe other places
>> suffer from the fact that they are hiding parts of the expressions
>> from the back-end, like SET_DESTs of single_set or the anatomy of
>> PARALELLs.
>>
>> Would it be in order to have a hook like the one attached?
>>
>> I am aware of that, in an ideal world, there wouldn't be more
>> than one hook to get rtx costs.  But well...
> 
> The number of hooks is not a problem.  The overall complexity of this
> interface (between the backends and optimisers; a group of related
> hooks) _is_ a problem.  Also, the interface should be good for all
> targets, easy to use, do one thing and do it well.

The interface is easy and straight forward, but the new hook might not
be as convenient as rtx_costs.  For example, testing for single_set is
a bit tedious when you don't have an insn.

There are situations where rtx_costs is called with outer_code = INSN;
cf. set_rtx_cost.

Using set_src_cost instead of a new hook that gracefully degrades
might make a hell break loose when rtx_costs just return some penalty
costs for unknown stuff instead of "0".

Therefore, a new hook is easier to use and understand, but it might
be confusing in what situations rtx_costs with outer_code = INSN is
used and in what situations patter_costs would be used.

Also not that set_src_cost is used incorrectly for set(zero_extract):
rtl.h states that it does "Return the cost of moving X into a register".
In rtlanal::insn_rtx_cost() however, this function is called for
*any* SETs on the SET_SRC, even if SET_DEST is not a register.

He might fix this by filtering out destinations that are zero_extract.
Subregs might be mem or regs, and the current implementation looks
good.

Moreover, insn_rtx_costs could do something like

rtx_cost (pat, VOIDmode, INSN, 4, speed_p)

when it sees a PARALLEL with more than 1 SET.

> Currently we have rtx_costs, which computes an estimated cost for any
> expression.  In most cases what we want to compute is the cost of an
> instruction though!  It can be a single set (in which case the
> expression cost is a reasonable approximation), but it also can be
> something else (like, a parallel).  Also, on many targets at least,
> it is *much* easier to compute the cost knowing that this is a valid
> instruction.
> 
> So I argue that we want to have a hook for insn cost.
> 
> Now what should it take as input?  An rtx_insn, or just the pattern
> (as insn_rtx_cost does)?  And if an rtx_insn, should that than be an
> rtx_insn that is already linked into the insn chain (so we can see what
> other insns generate its inputs, and which of its outputs are unused,
> etc.)?

IMO, if we have an insn then we could also pass it, it doesn't cost
anything.  However I don't know if the insn might have some strange
properties like an INSN_CODE that doesn't reflect the actual pattern.
Ans if reg_notes might be helpful and correct resp. up to date.

It should be in order to run the hook from within a sequence, hence
"insn chain" might not be what the user expects, i.e. prev_insn might
be NULL even though the current function already has some insns.


> One comment about your patch:
> 
>> +/* A magic value to be returned by TARGET_INSN_COSTS to indicate that
>> +   the costs are not known or too complicated to work out there.  */
>> +#define INSN_COSTS_UNKNOWN (-1234567)
> 
> Why not just -1?  And is 0 really so terrible; in the extremely rare
> case we really want cost 0, it won't hurt much saying "unknown", as we
> do currently.  For "hook unimplemented", just set the hook to NULL.
> 
> Segher

Using "0" is really bad design.  It's not clear when you are reading the
sources that it is some magic value.  Some identifier to refer to such
a value is much better.  And 0 is also bad because there might be
situations where 0 is a legal cost.  There might even be situations
where you want negative cost in order to prefer a specific pattern
re. some alternative (cost functions are not always 100% correct and
are expected costs as they run before reg alloc etc.)

My second proposal avoided magic value altogether and uses a bool*
instead.  My proposal below is similar, but is passes the cost
in a int*.  As the call site will have to check whether the hook
computed the costs, it might be more convenient to return whether
or not the hook came up with the costs or not.

Johann

-------------- next part --------------
A non-text attachment was scrubbed...
Name: insn-costs-v3.diff
Type: text/x-patch
Size: 3995 bytes
Desc: not available
URL: <https://gcc.gnu.org/pipermail/gcc/attachments/20170717/ca68355f/attachment.bin>