Re: [PATCH], Patch #2 of 10, Add RTL prefixed attribute

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

Hi Mike,

Some comments on this patch:

On Wed, Aug 14, 2019 at 05:59:13PM -0400, Michael Meissner wrote:
> Due to some of the existing load and store insns not using the traditional
> operands[0] and operands[1], the functions that test whether an insn is
> prefixed only use the insn and not the operands directly.

Both the "update" and the "indexed" attributes have no problem with
this: the insns that have the problem set the attribute value directly.
This is mainly all the various update insns, but there are a bunch more,
and they all need different settings for their attributes.

> 	* config/rs6000/rs6000.c (rs6000_emit_move): Add support for
> 	loading up pc-relatve addresses.

(typo btw)

> +void
> +rs6000_final_prescan_insn (rtx_insn *insn)
> +{
> +  next_insn_prefixed_p = (get_attr_prefixed (insn) != PREFIXED_NO);
> +  return;
> +}

Don't say "return;" at the end of a function please.

> +void
> +rs6000_asm_output_opcode (FILE *stream, const char *)
> +{
> +  if (next_insn_prefixed_p)
> +    {
> +      next_insn_prefixed_p = false;
> +      fprintf (stream, "p");
> +    }

You don't need to clear the flag here; the next call to
rs6000_final_prescan_insn will.

> +#define FINAL_PRESCAN_INSN(INSN, OPERANDS, NOPERANDS)			\
> +do									\
> +  {									\
> +    if (TARGET_PREFIXED_ADDR)						\
> +      rs6000_final_prescan_insn (INSN);					\
> +  }									\
> +while (0)

Either have the function only do what it needs to for prefixed, and call
it something with prefixed in the name, or put the TARGET_PREFIXED_ADDR
test in the function itself.

> +;; Load up a pc-relative address.  ASM_OUTPUT_OPCODE will emit the initial "p".
> +(define_insn "*pcrel_addr"
> +  [(set (match_operand:DI 0 "gpc_reg_operand" "=b*r")
> +	(match_operand:DI 1 "pcrel_address"))]
> +  "TARGET_PCREL"
> +  "la %0,%a1"
> +  [(set_attr "prefixed" "yes")])

(use P for addresses please)

> +;; Load up a pc-relative address to an external symbol.  If the symbol and the
> +;; program are both defined in the main program, the linker will optimize this
> +;; to a PADDI.  Otherwise, it will create a GOT address that is relocated by
> +;; the dynamic linker and loaded up.
> +(define_insn "*pcrel_ext_addr"
> +  [(set (match_operand:DI 0 "gpc_reg_operand" "=b*r")
> +	(match_operand:DI 1 "pcrel_external_address"))]
> +  "TARGET_PCREL"
> +  "ld %0,%a1"
> +  [(set_attr "prefixed" "yes")])

pld does an indirection more than pla does, but this is not clear at all
from the RTL, from the predicate names.  All this is *before* the linker
has done its thing, so pcrel_external_address is really some GOT memory,
so it should have that in its name.


Segher