This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


On Wed, Aug 28, 2019 at 12:14:58PM -0500, Segher Boessenkool wrote:
> 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.

Ok.

> > -(define_predicate "pcrel_external_address"
> > +(define_predicate "pcrel_ext_address"
> 
> That is a much worse name.  "ext" can mean "extended" or many more things.

Ok.

> 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.

Yeah, yeah, yeah.  I recall in Amsterdam there is the "Oude Kerk" (old church)
built in the 1200's and the "De Nieuwe Kerk" in Amsterdam (built in the 1500's)
and thinking then of the problems of calling something "new" and "old".


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

Ok.

> > -#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.

Ok, but I do hate things not lining up.  But I will keep the original masks as they are.

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

Actually no, they were created explicitly for the secondary reload handler when
I wrote this interface to add VSX support.  These masks were created for the
secondary reload handler to tell what type of addressing a mode has for the 3
hardware register classes (GPR, FPR, VMX) when you are given an address and
told to fix it up.

Now when I wrote them, I always meant to extend their use to the legitimate
address functions to replace all of the if this type is foo, then allow
increment or offset, etc.  I didn't get to other things as time, energy, and
deadlines came up.

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

Yes.

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

The flag is set to say this register class allows the funky (reg + reg) & -16
addressing used with the original Altivec instructions.  In retrospect, I
should have changed the Altivec memory to use an UNSPEC or some similar, but
that would be several years before we needed to clean that up.

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

It is used when the register is still a pseudo to tell the appropriate defaults
to use (in rs6000_secondary_reload_memory).  Before this patch, it used the any
field.

In the original code, the any register class was used by the mode_supports
helper function to say whether any one of the three reload register classes
supports a feature.

But even if one feature is used in one reload register class, for making
decisions, you really want to use the default class instead of the any class.
Otherwise, with direct move, it means that reload will often times do it in an
alternate register and do a direct move.  SDmode for example, supports offset
addresses in GPRs, but the normal usage you want to only use indexed addresses.

> 
> > 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.

The default addr mask is used in prefixed addressing to decide whether in the
abscense of the register class used to say whether D/DS/DQ-form instructions
would be used as non-prefixed instructions.

I can refactor it, but it will be a lot more code.

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

I can change it.

> > @@ -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.

No, in the context of the code, it means reload register class.  The whole
point is to reduce all of the normal register classes just to the 3 hardware
register types.

> 
> >    /* 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?

Actually I believe there are are only two places where I check for local and
external symbols.  Everywhere else only checks for local symbols (that is why
the previous patches used two booleans, to say whether you wanted local symbols
and/or external symbols).

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

I will look at 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).

Ok.

> > +;; 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?

Yes it is used in rs6000_emit_move.  Basically in the previous patch, you
complained about just creating the SET directly.  So I figured this was
clearer.  But I can go back to generating the SET and just add more comments,
and re-add the '*' in front of the name.'

> > +  [(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.

Yep.

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

Not really, but I will try.  However, then of course you have the issue that a
particular patch creates a function that isn't used for a few patches, and you
have to look at several patches all at once.

-- 
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meissner@linux.ibm.com, phone: +1 (978) 899-4797


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]