Re: [PATCH 4/5] MSP430: Implement TARGET_INSN_COST

[Segher Boessenkool <segher@kernel.crashing.org>]

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

> +  /* 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.


Segher