Hi Segher,

Thanks for having a look at the patch.

On Thu, Jul 23, 2020 at 01:34:22PM -0500, Segher Boessenkool wrote:
> Hi!
> 
> On Thu, Jul 23, 2020 at 04:56:14PM +0100, Jozef Lawrynowicz wrote:
> > +static int
> > +msp430_insn_cost (rtx_insn *insn, bool speed ATTRIBUTE_UNUSED)
> > +{
> > +  int cost;
> > +
> > +  if (recog_memoized (insn) < 0)
> > +    return 0;
> > +
> > +  cost = get_attr_length (insn);
> > +  if (TARGET_DEBUG_INSN_COSTS)
> > +    {
> > +      fprintf (stderr, "cost %d for insn:\n", cost);
> > +      debug_rtx (insn);
> > +    }
> > +
> > +  /* The returned cost must be relative to COSTS_N_INSNS (1). An insn with a
> > +     length of 2 bytes is the smallest possible size and so must be equivalent
> > +     to COSTS_N_INSNS (1).  */
> > +  return COSTS_N_INSNS (cost) / (2 * COSTS_N_INSNS (1));
> 
> This is the same as "cost / 2", so "length / 2" here, which doesn't look
> right.  The returned value should have the same "unit" as COSTS_N_INSNS
> does, so maybe you want  COSTS_N_INSNS (length / 2)  ?

Indeed it looks like I made a thinko in that calculation in TARGET_INSN_COSTS;
trying to make it verbose to show the thought behind the calculation backfired
:)

Fixing it to return "COSTS_N_INSNS (length / 2)" actually made codesize
noticeably worse for most of my benchmarks.
I had to define BRANCH_COST to indicate branches are not cheap.

In the original patch a cheap instruction would have a cost of 1.
When using the default BRANCH_COST of 1 to calculate the cost of a branch
compared to an insn (e.g. in ifcvt.c), BRANCH_COST would be wrapped in
COSTS_N_INSNS, scaling the cost to 4, which suitably disparaged
it vs the cheap insn cost of 1.

With the fixed insn_cost calculation, a cheap instruction would cost 4
with the COSTS_N_INSNS scaling, and a branch would cost the same, which is not
right.

Fixed in the attached patch.

> 
> > +  /* FIXME Add more detailed costs when optimizing for speed.
> > +     For now the length of the instruction is a good approximiation and roughly
> > +     correlates with cycle cost.  *
> 
> COSTS_N_INSNS (1) is 4, so that you can make things cost 5, 6, 7 to be a
> cost intermediate to COSTS_N_INSNS (1) and COSTS_N_INSNS (2).  This is
> very useful, scaling down the costs destroys that.
> 
> > +mdebug-insn-costs
> > +Target Report Mask(DEBUG_INSN_COSTS)
> > +Print insns and their costs as calculated by TARGET_INSN_COSTS.
> 
> It is already printed in the generated asm with -dp?  Not sure if you
> want more detail than that.
> 
>      '-dp'
>           Annotate the assembler output with a comment indicating which
>           pattern and alternative is used.  The length and cost of each
>           instruction are also printed.
> 

During development I found it useful to see the insns in RTL format and their
costs alongside that.  In hindsight, it doesn't really have much use in the
finalized patch, so I've removed it.

Thanks!
Jozef

> 
> Segher