Re: [PATCH, V3, #3 of 10], Add prefixed RTL insn attribute

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

Hi Mike,

On Mon, Aug 26, 2019 at 04:31:02PM -0400, Michael Meissner wrote:
> 	(rs6000_asm_output_opcode): New function for prifixed memory.

Typo.  Just say "New." or "New function." please.

> --- gcc/config/rs6000/rs6000.c	(revision 274871)
> +++ gcc/config/rs6000/rs6000.c	(working copy)
> @@ -13827,23 +13827,23 @@ addr_mask_to_trad_insn (machine_mode mod
>       early RTL stages before register allocation has been done.  */
>    if ((addr_mask & flags) == RELOAD_REG_MULTIPLE)
>      {
> -      machine_mode inner = word_mode;
> +      machine_mode mode2 = mode;

So what is "mode2" for?  A meaningful name and/or some comments would help.

> +	  if ((reg_addr[E_DFmode].default_addr_mask & RELOAD_REG_OFFSET) != 0)
> +	    mode = DFmode;

(Don't use E_ if you do not need it -- i.e. most of the time).

> +/* Helper function to take a REG and a MODE and turn it into the traditional
> +   instruction format (D/DS/DQ) used for offset memory.  */

Is this the form of the preferred insn to do this?  Or ths minimum required
to do it at all?  Something else?

> +  /* If it isn't a register, use the defaults.  */
> +  if (!REG_P (reg) && !SUBREG_P (reg))
> +    addr_mask = reg_addr[mode].default_addr_mask;
> +
> +  else
> +    {
> +      unsigned int r = reg_or_subregno (reg);

This ICEs if it is a subreg of something else than a reg.

You can just start with

  if (SUBREG_P (reg))
    reg = SUBREG_REG (reg);

  if (REG_P (reg))
   ... etc.

> +/* Whether a load instruction is a prefixed instruction.  This is called from
> +   the prefixed attribute processing.  */
> +
> +bool
> +prefixed_load_p (rtx_insn *insn)
> +{
> +  /* Validate the insn to make sure it is a normal load insn.  */
> +  extract_insn_cached (insn);
> +  if (recog_data.n_operands < 2)
> +    return false;

Why don't you handle this the same way "indexed" and "update" are already
handled?  That is *easy* and it *works*, it trivially verifiably works.
It also doesn't care whether something is a load or a store.  You hardcode
the few exceptions (okay, twenty or whatever update insns -- but all are
similar, so that is easy), and everything else just works.

The way you code it you just hope to exclude all of the exceptions,
instead of handling them directly.

> +void
> +rs6000_asm_output_opcode (FILE *stream)
> +{
> +  if (next_insn_prefixed_p)
> +    fputc ('p', stream);
> +
> +  return;
> +}

You can just write fprintf fwiw, the compile can optimise it for you
just fine.

> +#define ASM_OUTPUT_OPCODE(STREAM, OPCODE)				\
> +  do									\
> +    {									\
> +     if (TARGET_PREFIXED_ADDR)						\
> +       rs6000_asm_output_opcode (STREAM);				\
> +    }									\
> +  while (0)

(Indentation of the "if" is weird?)

> +;; Whether an insn is a prefixed insn, and an initial 'p' should be printed
> +;; before the instruction.  A prefixed instruction has a prefix instruction

Whether it is a prefixed insn, period.

> +;; word that extends the immediate value of the instructions from 12-16 bits to
> +;; 34 bits.  The macro ASM_OUTPUT_OPCODE emits a leading 'p' for prefixed
> +;; insns.  The default "length" attribute will also be adjusted by default to
> +;; be 12 bytes.

Don't say all the effects here, say that where you make it happen?

> +;; Length in bytes of instructions that use prefixed addressing and length in
> +;; bytes of instructions that does not use prefixed addressing.  This allows
> +;; both lengths to be defined as constants, and the length attribute can pick
> +;; the size as appropriate.
> +(define_attr "prefixed_length" "" (const_int 12))
> +(define_attr "non_prefixed_length" "" (const_int 4))

Do you mean a define_insn can override either to something else?  Then
say that, please?

> +;; Length of the instruction (in bytes).  Prefixed insns are 8 bytes, but the
> +;; assembler might issue need to issue a NOP so that the prefixed instruction
> +;; does not cross a cache boundary, which makes them possibly 12 bytes.

s/issue //

> @@ -9883,8 +9926,8 @@ (define_insn "pcrel_local_addr"
>    [(set (match_operand:DI 0 "gpc_reg_operand" "=b*r")
>  	(match_operand:DI 1 "pcrel_local_address"))]
>    "TARGET_PCREL"
> -  "pla %0,%a1"
> -  [(set_attr "length" "12")])
> +  "la %0,%a1"
> +  [(set_attr "prefixed" "yes")])

And just like this you can set the few insns that do not have operands 0
and 1 as source and dest to "no", exactly like is already done for "update"
and "indexed".


Segher