[PATCH V3, #1 of 10], Add basic pc-relative support

[Segher Boessenkool]

Hi Mike,

On Mon, Aug 26, 2019 at 03:54:14PM -0400, Michael Meissner wrote:
> @@ -1626,8 +1626,8 @@ (define_predicate "small_toc_ref"
>    return GET_CODE (op) == UNSPEC && XINT (op, 1) == UNSPEC_TOCREL;
>  })
>  
> -;; Return true if the operand is a pc-relative address.
> -(define_predicate "pcrel_address"
> +;; Return true if the operand is a pc-relative address to a local label.
> +(define_predicate "pcrel_local_address"
>    (match_code "label_ref,symbol_ref,const")

Not just to a local label?  Please fix the comment.

> -(define_predicate "pcrel_external_address"
> +(define_predicate "pcrel_ext_address"

That is a much worse name.  "ext" can mean "extended" or many more things.

Having good names for these very basic things is important.  The names
shape the concepts, how you think about the code.

> +/* Enumeration giving the type of traditional addressing that would be used to
> +   decide whether an instruction uses prefixed memory or not.  If the
> +   traditional instruction uses the DS instruction format, and the bottom 2
> +   bits of the offset are not 0, the traditional instruction cannot be used,
> +   but a prefixed instruction can be used.  */

"Traditional" is a bad word for documentation.  What you mean is what was
supported before.  Before you know it "new" will be old as well.

What you mean is non-prefixed addressing?  Please say so, then.

(And it is "form", not "format", in all the insn descriptions etc...
Like "D-form".  Please use the same.)

> -#define RELOAD_REG_VALID	0x01	/* Mode valid in register..  */
> -#define RELOAD_REG_MULTIPLE	0x02	/* Mode takes multiple registers.  */
> -#define RELOAD_REG_INDEXED	0x04	/* Reg+reg addressing.  */
> -#define RELOAD_REG_OFFSET	0x08	/* Reg+offset addressing. */
> -#define RELOAD_REG_PRE_INCDEC	0x10	/* PRE_INC/PRE_DEC valid.  */
> -#define RELOAD_REG_PRE_MODIFY	0x20	/* PRE_MODIFY valid.  */
> -#define RELOAD_REG_AND_M16	0x40	/* AND -16 addressing.  */
> -#define RELOAD_REG_QUAD_OFFSET	0x80	/* quad offset is limited.  */
> +#define RELOAD_REG_VALID	0x001	/* Mode valid in register..  */
> +#define RELOAD_REG_MULTIPLE	0x002	/* Mode takes multiple registers.  */
> +#define RELOAD_REG_INDEXED	0x004	/* Reg+reg addressing.  */
> +#define RELOAD_REG_OFFSET	0x008	/* Reg+offset addressing. */
> +#define RELOAD_REG_PRE_INCDEC	0x010	/* PRE_INC/PRE_DEC valid.  */
> +#define RELOAD_REG_PRE_MODIFY	0x020	/* PRE_MODIFY valid.  */
> +#define RELOAD_REG_AND_M16	0x040	/* AND -16 addressing.  */
> +#define RELOAD_REG_QUAD_OFFSET	0x080	/* DQ offset (bottom 4 bits 0).  */
> +#define RELOAD_REG_DS_OFFSET	0x100	/* DS offset (bottom 2 bits 0).  */

As explained before, do not do this.  Do not use 3-digit hex numbers; don't
change existing definitions for no reason, not in the middle of an unrelated
patch, either.

> @@ -370,6 +369,8 @@ struct rs6000_reg_addr {

Can you fix this struct / arrays / whatever, instead of adding more to it?

>    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.  */
>    addr_mask_type addr_mask[(int)N_RELOAD_REG]; /* Valid address masks.  */
> +  addr_mask_type any_addr_mask;		/* OR of GPR/FPR/VMX addr_masks.  */
> +  addr_mask_type default_addr_mask;	/* Default addr_mask to use.  */

And these "address masks" are bitmaps of random flags, one for each
"register class" (which is not related to the core GCC concept of "register
class", and the bits are called "RELOAD_REG_*" although this isn't for
reload at all?


Don't pile new stuff on without cleaning up the old stuff first.


> +	  /* 64-bit and larger values on GPRs need DS format instructions.  All

"Need"...  Well you want to use ld insns, please just say that?

> +	     non-vector offset instructions in Altivec registers need the DS
> +	     format instructions.  */

And this is talking about lxsd / lxssp?

> +	  const addr_mask_type quad_flags = (RELOAD_REG_OFFSET
> +					     | RELOAD_REG_QUAD_OFFSET);
> +
> +	  if ((addr_mask & quad_flags) == RELOAD_REG_OFFSET
> +	      && ((rc == RELOAD_REG_GPR && msize >= 8 && TARGET_POWERPC64)
> +		  || (rc == RELOAD_REG_VMX)))
> +	    addr_mask |= RELOAD_REG_DS_OFFSET;
> +
>  	  reg_addr[m].addr_mask[rc] = addr_mask;
> -	  any_addr_mask |= addr_mask;
> +	  any_addr_mask |= (addr_mask & ~RELOAD_REG_AND_M16);

Why do you need this last line?  Why was that flag set at all?  What does
"any mask" mean if it is not?

> +      /* Figure out what the default reload register set that should be used
> +	 for each mode,

"Figure out what register set should be used by default for each mode"?

And what does reload have to do with it?

> that should mirror the expected usage (i.e. vectors in
> +	 vector registers, ints in GPRs, etc).  Fall back to GPRs as a last
> +	 resort if the mode isn't valid in the vector/floating point registers.
> +	 In the case of vectors and FP, we want to test the reload register
> +	 classes in the order of epxected use or in terms of functionality (the

expected

> +	 FPRs offer offsettable loads/stores in earlier ISAs).  */

"If targetting ISA 2.07 or before, we need FPRs if we need to do a floating
point load using offset addressing".  Something like that?

> +      int def_rc;
> +      int rc_order[2];
> +      int rc_max = 0;
> +
> +      /* IEEE 128-bit hardware floating point insns use Altivec registers.  */
> +      if (TARGET_FLOAT128_HW && FLOAT128_IEEE_P (m))
> +	rc_order[rc_max++] = RELOAD_REG_VMX;
> +
> +      /* Normal vectors and software IEEE 128-bit can use either floating point
> +	 registers or Altivec registers.  */
> +      else if (TARGET_VSX && (VECTOR_MODE_P (m) || FLOAT128_IEEE_P (m)))
> +	{
> +	  rc_order[rc_max++] = RELOAD_REG_FPR;
> +	  rc_order[rc_max++] = RELOAD_REG_VMX;
> +	}
> +
> +      /* Altivec only vectors use the Altivec registers.  */
> +      else if (TARGET_ALTIVEC && !TARGET_VSX && VECTOR_MODE_P (m))
> +	rc_order[rc_max++] = RELOAD_REG_VMX;
> +
> +      /* For scalar binary/decimal floating point, prefer FPRs over altivec
> +	 registers.  */
> +      else if (TARGET_HARD_FLOAT && SCALAR_FLOAT_MODE_P (m))
> +	{
> +	  rc_order[rc_max++] = RELOAD_REG_FPR;
> +	  rc_order[rc_max++] = RELOAD_REG_VMX;
> +	}
> +
> +      /* Default to GPRs if neither FPRs or Altivec registers is valid and
> +	 preferred.  */
> +      def_rc = RELOAD_REG_GPR;
> +      for (int i = 0; i < rc_max; i++)
> +	{
> +	  int rc_num = rc_order[i];
> +	  if ((reg_addr[m].addr_mask[rc_num] & RELOAD_REG_VALID) != 0)
> +	    {
> +	      def_rc = rc_num;
> +	      break;
> +	    }
> +	}
> +
> +      reg_addr[m].default_addr_mask = (reg_addr[m].addr_mask[def_rc]
> +				       & ~RELOAD_REG_AND_M16);

Please factor this better.  You don't need a "default result code" variable
either, then.

> @@ -9634,6 +9704,21 @@ rs6000_emit_move (rtx dest, rtx source,
>  	  return;
>  	}
>  
> +      /* Handle loading up pc-relative addresses.  */
> +      if (TARGET_PCREL && mode == E_DImode)

(Why does this need E_, btw?)

> @@ -10770,11 +10855,10 @@ rs6000_secondary_reload_memory (rtx addr
>  		 & ~RELOAD_REG_AND_M16);
>  
>    /* If the register allocator hasn't made up its mind yet on the register
> -     class to use, settle on defaults to use.  */
> +     class to use, use the default address mask bits.  */
>    else if (rclass == NO_REGS)

And this *does* mean register class.

>    /* Is it a pc-relative address?  */
> -  else if (pcrel_address (x, Pmode))
> +  else if (pcrel_local_address (x, Pmode) || pcrel_ext_address (x, Pmode))

This sounds like something you want to test together all over the place.
Please make a helper function?

> +  /* If the mode does not support offset addressing directly, but it has
> +     multiple registers, see if we can figure out a type that after splitting
> +     the load/store, will be used (i.e. for a vector, use the element, for IBM
> +     long double or TDmode use DFmode, etc.).  This is typically needed in the
> +     early RTL stages before register allocation has been done.  */

s/type/mode/

> +  if ((addr_mask & flags) == RELOAD_REG_MULTIPLE)
> +    {
> +      machine_mode inner = word_mode;
> +
> +      if (COMPLEX_MODE_P (mode))
> +	{
> +	  inner = GET_MODE_INNER (mode);
> +	  if ((reg_addr[inner].default_addr_mask & RELOAD_REG_OFFSET) == 0)
> +	    inner = word_mode;
> +	}
> +
> +      if (FLOAT128_2REG_P (mode))
> +	{
> +	  inner = DFmode;
> +	  if ((reg_addr[inner].default_addr_mask & RELOAD_REG_OFFSET) == 0)
> +	    inner = word_mode;
> +	}
> +
> +      addr_mask = reg_addr[inner].default_addr_mask;
> +    }

Are these the only two cases?  Is there some better way to determine this?

You don't handle vectors, contrary to the comment.

The word_mode thing needs a comment (it isn't clear to me what it is really
trying to do).

I think this would all be much simpler with just a few lines of code instead
of all these tables, fwiw.

>  bool
> -rs6000_prefixed_address_mode_p (rtx addr, machine_mode mode)
> +prefixed_local_addr_p (rtx addr,
> +		       machine_mode mode,
> +		       trad_insn_type trad_insn)
>  {
> -  if (!TARGET_PREFIXED_ADDR || !mode_supports_prefixed_address_p (mode))
> +  /* Don't allow SDmode, because it only can be loaded into FPRs using LFIWZX
> +     instruction.  */
> +  if (!TARGET_PREFIXED_ADDR || mode == E_SDmode)
>      return false;

Write this as two separate conditionals please, the two are unrelated (and
the comment is only about the second).

> +;; Load up a pc-relative address.  Print_operand_address will append a @pcrel
> +;; to the symbol or label.
> +(define_insn "pcrel_local_addr"

This isn't used anywhere?  Not by name, that is?

> +  [(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")])

I wonder if that whole "b*r" thing is useful at all these days, btw.


This patch changes a whole bunch of things.  You probably can split it
into smaller, self-contained pieces.


Segher