Re: [PATCH], Patch #1 replacement (fix issues with future TLS patches)

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

Hi Mike,

On Thu, Aug 15, 2019 at 05:19:16PM -0400, Michael Meissner wrote:
> -;; Return true if the operand is a pc-relative address.
> +;; Return true if the operand is a pc-relative address to a local symbol.

The pcrel_addr_p comment says it is *not* just for local symbols.  So
which is it?

Having both something called "pcrel_address" and something called
"pcrel_addr_p" is confusing.  And "pcrel_addr_p" isn't a predicate at
all, so it should not be called that.  Or you can remove that "info"
argument, which is probably a good idea.

>  (define_predicate "pcrel_address"
>    (match_code "label_ref,symbol_ref,const")
>  {
> +  return pcrel_addr_p (op, true, false, PCREL_NULL);
>  })

Please avoid boolean arguments altogether; it isn't clear at all what
they mean here.

Ah, they say only locals are allowed here.  So this RTL predicate
shouldn't be called "pcrel_address"; it should have "local" in the name
somewhere.

>  ;; Return 1 if op is a prefixed memory operand.
>  (define_predicate "prefixed_mem_operand"
>    (match_code "mem")
>  {
> -  return rs6000_prefixed_address_mode_p (XEXP (op, 0), GET_MODE (op));
> +  return prefixed_local_addr_p (XEXP (op, 0), mode, INSN_FORM_UNKNOWN);
>  })

Similar issues with "local" here.

>  (define_predicate "pcrel_external_mem_operand"
>    (match_code "mem")
>  {
> -  return pcrel_external_address (XEXP (op, 0), Pmode);
> +  return pcrel_addr_p (XEXP (op, 0), false, true, PCREL_NULL);
>  })

Why this change?

> +/* Pc-relative address broken into component parts by pcrel_addr_p.  */
> +typedef struct {
> +  rtx base_addr;		/* SYMBOL_REF or LABEL_REF.  */
> +  HOST_WIDE_INT offset;		/* Offset from the base address.  */
> +  bool external_p;		/* Is the symbol external?  */
> +} pcrel_info_type;

Don't use typedefs please.

Don't call booleans xxx_p; xxx_p is a name used for a predicate, that
is, a pure (or "const" in GCC terms) function returning a boolean.

Don't name types "*_type".

> +#define PCREL_NULL ((pcrel_info_type *)0)

Please just use NULL where you use this.  (Or 0 as far as I care, but
that's not the GCC coding style :-) ).

> --- gcc/config/rs6000/rs6000.c	(revision 274172)
> +++ gcc/config/rs6000/rs6000.c	(working copy)
> @@ -369,8 +369,11 @@ struct rs6000_reg_addr {
>    enum insn_code reload_fpr_gpr;	/* INSN to move from FPR to GPR.  */
>    enum insn_code reload_gpr_vsx;	/* INSN to move from GPR to VSX.  */
>    enum insn_code reload_vsx_gpr;	/* INSN to move from VSX to GPR.  */
> +  enum insn_form default_insn_form;	/* Default format for offsets.  */
> +  enum insn_form insn_form[(int)N_RELOAD_REG]; /* Register insn format.  */
>    addr_mask_type addr_mask[(int)N_RELOAD_REG]; /* Valid address masks.  */

Why the casts here?  Not all places use this cast, so why is it needed
here and not in all cases?

> +/* Return a character that can be printed out to describe an instruction
> +   format.  */
> +
> +DEBUG_FUNCTION char
> +rs6000_debug_insn_form (enum insn_form iform)
> +{
> +  char ret;
> +
> +  switch (iform)
> +    {
> +    case INSN_FORM_UNKNOWN:  ret = '-'; break;
> +    case INSN_FORM_D:        ret = 'd'; break;
> +    case INSN_FORM_DS:       ret = 's'; break;
> +    case INSN_FORM_DQ:       ret = 'q'; break;
> +    case INSN_FORM_X:        ret = 'x'; break;
> +    case INSN_FORM_PREFIXED: ret = 'p'; break;
> +    default:                 ret = '?'; break;
> +    }
> +
> +  return ret;
> +}

This doesn't follow the coding style.

> +  fprintf (stderr, "  Format: %c:%c%c%c",
> +          rs6000_debug_insn_form (reg_addr[m].default_insn_form),
> +          rs6000_debug_insn_form (reg_addr[m].insn_form[RELOAD_REG_GPR]),
> +          rs6000_debug_insn_form (reg_addr[m].insn_form[RELOAD_REG_FPR]),
> +          rs6000_debug_insn_form (reg_addr[m].insn_form[RELOAD_REG_VMX]));

Is this useful?  For others I mean, not just for you.

> +/* Set up the instruction format for each mode and register type from the
> +   addr_mask.  */
> +
> +static void
> +setup_insn_form (void)
> +{
> +  for (ssize_t m = 0; m < NUM_MACHINE_MODES; ++m)

Why ssize_t?  Most places just use int.

> +    {
> +      machine_mode scalar_mode = (machine_mode) m;
> +
> +      /* Convert complex and IBM double double/_Decimal128 into their scalar
> +	 parts that the registers will be split into for doing load or
> +	 store.  */
> +      if (COMPLEX_MODE_P (scalar_mode))
> +	scalar_mode = GET_MODE_INNER (scalar_mode);

Do you also need to handle some vector modes here?

> +      if (FLOAT128_2REG_P (scalar_mode))
> +	scalar_mode = DFmode;
> +
> +      for (ssize_t rc = FIRST_RELOAD_REG_CLASS; rc <= LAST_RELOAD_REG_CLASS; rc++)

(overwide line)

> +	{
> +	  machine_mode single_reg_mode = scalar_mode;
> +	  size_t msize = GET_MODE_SIZE (scalar_mode);
> +	  addr_mask_type addr_mask = reg_addr[scalar_mode].addr_mask[rc];
> +	  enum insn_form iform = INSN_FORM_UNKNOWN;
> +
> +	  /* Is the mode permitted in the GPR/FPR/Altivec registers?  */
> +	  if ((addr_mask & RELOAD_REG_VALID) != 0)
> +	    {
> +	      /* The addr_mask does not have the offsettable or indexed bits
> +		 set for modes that are split into multiple registers (like
> +		 IFmode).  It doesn't need this set, since typically by time it
> +		 is used in secondary reload, the modes are split into
> +		 component parts.
> +
> +		 The instruction format however can be used earlier in the
> +		 compilation, so we need to setup what kind of instruction can
> +		 be generated for the modes that are split.  */

If it's only "typically" split, is it true that the bits do not have to
be set?

Snip the rest of this function...  It needs much better factoring, and
better commenting, I cannot make heads or tails of it, sorry.

> +  /* Update the instruction formats.  */
> +  setup_insn_form ();

"Update"?  And s/format/form/ throughout.

>  void
>  print_operand_address (FILE *file, rtx x)
>  {
> +  pcrel_info_type pcrel_info;
> +
>    if (REG_P (x))
>      fprintf (file, "0(%s)", reg_names[ REGNO (x) ]);
>  
>    /* Is it a pc-relative address?  */
> -  else if (pcrel_address (x, Pmode))
> +  else if (pcrel_addr_p (x, true, true, &pcrel_info))

This is the only place that uses non-null for the info argument.  So
delete that from the normal function please, and handle that here, maybe
with some helper functions.

Don't do two (or more) different things in one function, in general.

> +/* Return true if the address ADDR is a prefixed address either with a large
> +   offset, an offset that does not fit in the normal instruction form, or a
> +   pc-relative address to a local symbol.

"Return true if ADDR is a local address that needs a prefix insn to
encode."?

You don't need to describe all exact reasons here...  That probably is
out-of-date (and/or not so exact) anyway.

>  
> +   MODE is the mode of the memory.
>  
> +   IFORM is used to determine if the traditional address is either DS format or
> +   DQ format and the bottom bits of the offset are non-zero.  */

s/traditional/non-prefixed/?  "Traditional" is a word like "old"; it is
stale almost before you write it.

>  bool
> +prefixed_local_addr_p (rtx addr, machine_mode mode, enum insn_form iform)

> +      if (!SIGNED_34BIT_OFFSET_P (offset))
> +	return false;

      /* Can this be described with a non-prefixed insn?  */
      if (!SIGNED_16BIT_OFFSET_P (offset))
	return true;

(and then the DS/DQ forms that need a prefix).

> +	  /* Use default if we don't know the precise instruction format.  */
> +	  if (iform == INSN_FORM_UNKNOWN)
> +	    iform = reg_addr[mode].default_insn_form;

So is this default as strict as possible?  As lenient as possible?  Or
what?

> +/* Given a register and a mode, return the instruction format for that
> +   register.  If the register is a pseudo register, use the default format.

What *is* the default form?  How is that decided?

Nowhere do you say if what something returns is as optimistic or as
pessimistic as possible.  Either option has something to say for it,
but it is important we know what it is :-)

> +enum insn_form
> +reg_to_insn_form (rtx reg, machine_mode mode)

What does this describe?  The insn form used for loading/storing that
register?

> +  /* If it isn't a register, use the defaults.  */
> +  if (!REG_P (reg) && !SUBREG_P (reg))
> +    iform = reg_addr[mode].default_insn_form;

If so...  How can that happen?

> +	  /* For anything else (SPR, CA, etc.) assume the GPR registers will be
> +	     used to load or store the value.  */

That's a big assumption.  gcc_unreachable instead?


Segher