V4, patch #1: Rework prefixed/pc-relative lookup

This patch reworks the prefixed and pc-relative memory matching functions.

As I said in the intro message, I do not re-use the address mask bits in
reg_addr, but instead, I have a separate function that takes an address and
decodes it into the various different flavors (single register address, D-form
16-bit address, X-form indexed address, numeric 34-bit offset, local
pc-relative address, etc.).  The caller then decides whether the address
matches what they are looking for.

I have two enumerations that I added to this series:

    1)	insn_form: This is the address format (D, DS, DQ, X, etc.);

    2)	non_prefixed: This is a limited enum that just describes the format of
	the non-prefixed instruction to decide if an address needs to be
	prefixed or not.

Originally, I was trying to re-use the same insn_form enumeration for both the
output and the input to say what the traditional instruction uses, but I
ultimately separated them to make it clearer.

This is an infrastructure patch.  It needs the second patch to enable basic
pc-relative support.

I have done a bootstrap build with all of the patches applied, and there were
no regressions in the test suite.  After posting these patches, I will start a
job to build each set of patches in turn just to make sure there are no extra
warnings.

Can I commit this patch to the trunk?

2019-09-18  Michael Meissner  <meissner@linux.ibm.com>

	* config/rs6000/predicates.md (pcrel_address): Delete predicate.
	(pcrel_local_address): Replace pcrel_address predicate, use the
	new function address_to_insn_form.
	(pcrel_external_address): Replace with new implementation using
	address_to_insn_form..
	(prefixed_mem_operand): Delete predicate which is now unused.
	(pcrel_external_mem_operand): Delete predicate which is now
	unused.
	* config/rs6000/rs6000-protos.h (enum insn_form): New
	enumeration.
	(enum non_prefixed): New enumeration.
	(address_to_insn_form): New declaration.
	* config/rs6000/rs6000.c (print_operand_address): Check for either
	pc-relative local symbols or pc-relative external symbols.
	(mode_supports_prefixed_address_p): Delete, no longer used.
	(rs6000_prefixed_address_mode_p): Delete, no longer used.
	(address_to_insn_form): New function to decode an address format.

Hi Mike,

On Wed, Sep 18, 2019 at 07:49:18PM -0400, Michael Meissner wrote:
> This patch reworks the prefixed and pc-relative memory matching functions.

This mostly looks fine, thanks!  A few smaller things:


> 	(pcrel_external_address): Replace with new implementation using
> 	address_to_insn_form..

(Two dots is one too many).


> +(define_predicate "pcrel_local_or_external_address"
> +  (match_code "label_ref,symbol_ref,const")
> +{
> +  enum insn_form iform = address_to_insn_form (op, mode, NON_PREFIXED_DEFAULT);
> +  return (iform == INSN_FORM_PCREL_EXTERNAL || iform == INSN_FORM_PCREL_LOCAL);
> +})

(define_predicate "pcrel_local_or_external_address"
  (ior (match_operand 0 "pcrel_local_address")
       (match_operand 0 "pcrel_external_address")))

(or similar) please.  genpreds will generate effectively the same code
as you had automatically.


> +/* Different PowerPC instruction formats that are used by GCC.  There are
> +   various other instruction formats used by the PowerPC hardware, but the
> +   these formats are not currently used by GCC.  */
> +
> +enum insn_form {
> +  INSN_FORM_BAD,		/* Bad instruction format.  */
> +  INSN_FORM_BASE_REG,		/* Base register only.  */
> +  INSN_FORM_D,			/* Base register + 16-bit numeric offset.  */
> +  INSN_FORM_DS,			/* Base register + 14-bit offset + 00.  */
> +  INSN_FORM_DQ,			/* Base register + 12-bit offset + 0000.  */

It may be easier to describe DS-form as "D-form, with the offset aligned
to a (single) word" and DQ-form as "D-form, with the offset aligned to a
quad-word".  (Or what you do below; see below).

> +  INSN_FORM_X,			/* Base register + index register.  */
> +  INSN_FORM_UPDATE,		/* Address udpates base register.  */

(typo, "updates").

> +  INSN_FORM_LO_SUM,		/* Special offset instruction.  */

That's a somewhat lame description :-)  It's not really a separate form
insn anyway, hrm.  Can you come up with a better comment?  I have no
suggestions, so yeah maybe just keep it like you have.


> +  INSN_FORM_PREFIXED_NUMERIC,	/* Base register + 34 bit numeric offset.  */
> +  INSN_FORM_PCREL_LOCAL,	/* Pc-relative local symbol.  */
> +  INSN_FORM_PCREL_EXTERNAL	/* Pc-relative external symbol.  */
> +};

Either pc or PC please.  It's an initialism.


> +/* Instruction format for the non-prefixed version of a load or store.  This is
> +   used to determine if a 16-bit offset is valid to be used with a non-prefixed
> +   (traditional) instruction or if the bottom bits of the offset cannot be used
> +   with a DS or DQ instruction format, and GCC has to use a prefixed
> +   instruction for the load or store.  */
> +
> +enum non_prefixed {
> +  NON_PREFIXED_DEFAULT,		/* Use the default.  */
> +  NON_PREFIXED_D,		/* All 16-bits are valid.  */
> +  NON_PREFIXED_DS,		/* Bottom 2 bits must be 0.  */
> +  NON_PREFIXED_DQ,		/* Bottom 4 bits must be 0.  */
> +  NON_PREFIXED_X		/* No offset memory form exists.  */
> +};

Yeah the DS- and DQ-form descriptions here are nicer I think, thanks.

Maybe non_prefixed_form is a clearer name?  But it is longer of course.
You decide.


> +      if (SYMBOL_REF_P (x) && !SYMBOL_REF_LOCAL_P (x))
> +	fputs ("@got", file);
> +
>        fputs ("@pcrel", file);

I'd just use fprintf btw, GCC knows since decades to optimise that to
fputs, and it is easier to read IMO.  Not that fputs is so super bad,
but every little we do not have to think helps (helps us think, think
about more important matters!)


> +/* Given an address (ADDR), a mode (MODE), and what the format of the
> +   non-prefixed address (NON_PREFIXED_INSN) is, return the instruction format
> +   for the address.  */
> +
> +enum insn_form
> +address_to_insn_form (rtx addr,
> +		      machine_mode mode,
> +		      enum non_prefixed non_prefixed_insn)

non_prefixed_form, instead?


> +{
> +  rtx op0, op1;

You can declare these at first use.  Declaring things in multiple blocks
(so with shorter scopes) is a bit nicer.

> +  /* If we don't support offset addressing, make sure only indexed addressing
> +     is allowed.  We special case SDmode so that the register allocator does
> +     try to move SDmode through GPR registers, but instead uses the 32-bit
> +     integer read/write instructions for the floating point registers.  */

Does *not* try?

Read/write, do you mean load/store?  lfiwzx and friends?


> +  if (GET_CODE (addr) != PLUS)
> +    return GET_CODE (addr) == LO_SUM ? INSN_FORM_LO_SUM : INSN_FORM_BAD;

  if (GET_CODE (addr) == LO_SUM)
    return INSN_FORM_LO_SUM;

  if (GET_CODE (addr) != PLUS)
    return INSN_FORM_BAD;

(Avoid using the conditional operator if you can use an "if" statement
just as well; easier to read).

> +  op0 = XEXP (addr, 0);
> +  op1 = XEXP (addr, 1);
> +
> +  if (REG_P (op1) || SUBREG_P (op1))
> +    return INSN_FORM_X;

I think you should have checked op0 here as well?

> +  /* If it isn't pc-relative, check for 16-bit D/DS/DQ-form.  */
> +  if (!REG_P (op0) && !SUBREG_P (op0))
> +    return INSN_FORM_BAD;

(Instead of only later here).

Overall this looks like it will work nicely, thanks!


Segher

On Fri, Sep 20, 2019 at 08:29:04PM -0500, Segher Boessenkool wrote:
> Hi Mike,
> 
> On Wed, Sep 18, 2019 at 07:49:18PM -0400, Michael Meissner wrote:
> > This patch reworks the prefixed and pc-relative memory matching functions.
> 
> This mostly looks fine, thanks!  A few smaller things:
> 
> 
> > 	(pcrel_external_address): Replace with new implementation using
> > 	address_to_insn_form..
> 
> (Two dots is one too many).

Yes.

> > +(define_predicate "pcrel_local_or_external_address"
> > +  (match_code "label_ref,symbol_ref,const")
> > +{
> > +  enum insn_form iform = address_to_insn_form (op, mode, NON_PREFIXED_DEFAULT);
> > +  return (iform == INSN_FORM_PCREL_EXTERNAL || iform == INSN_FORM_PCREL_LOCAL);
> > +})
> 
> (define_predicate "pcrel_local_or_external_address"
>   (ior (match_operand 0 "pcrel_local_address")
>        (match_operand 0 "pcrel_external_address")))
> 
> (or similar) please.  genpreds will generate effectively the same code
> as you had automatically.

Ok.

> > +/* Different PowerPC instruction formats that are used by GCC.  There are
> > +   various other instruction formats used by the PowerPC hardware, but the
> > +   these formats are not currently used by GCC.  */
> > +
> > +enum insn_form {
> > +  INSN_FORM_BAD,		/* Bad instruction format.  */
> > +  INSN_FORM_BASE_REG,		/* Base register only.  */
> > +  INSN_FORM_D,			/* Base register + 16-bit numeric offset.  */
> > +  INSN_FORM_DS,			/* Base register + 14-bit offset + 00.  */
> > +  INSN_FORM_DQ,			/* Base register + 12-bit offset + 0000.  */
> 
> It may be easier to describe DS-form as "D-form, with the offset aligned
> to a (single) word" and DQ-form as "D-form, with the offset aligned to a
> quad-word".  (Or what you do below; see below).

Ok.

> > +  INSN_FORM_X,			/* Base register + index register.  */
> > +  INSN_FORM_UPDATE,		/* Address udpates base register.  */
> 
> (typo, "updates").
> 
> > +  INSN_FORM_LO_SUM,		/* Special offset instruction.  */
> 
> That's a somewhat lame description :-)  It's not really a separate form
> insn anyway, hrm.  Can you come up with a better comment?  I have no
> suggestions, so yeah maybe just keep it like you have.

None of my code uses INSN_FORM_LO_SUM, but I was adding it for completeness,
since it is a valid instruction format (as used within the compiler).

> 
> > +  INSN_FORM_PREFIXED_NUMERIC,	/* Base register + 34 bit numeric offset.  */
> > +  INSN_FORM_PCREL_LOCAL,	/* Pc-relative local symbol.  */
> > +  INSN_FORM_PCREL_EXTERNAL	/* Pc-relative external symbol.  */
> > +};
> 
> Either pc or PC please.  It's an initialism.

Yep.

> 
> > +/* Instruction format for the non-prefixed version of a load or store.  This is
> > +   used to determine if a 16-bit offset is valid to be used with a non-prefixed
> > +   (traditional) instruction or if the bottom bits of the offset cannot be used
> > +   with a DS or DQ instruction format, and GCC has to use a prefixed
> > +   instruction for the load or store.  */
> > +
> > +enum non_prefixed {
> > +  NON_PREFIXED_DEFAULT,		/* Use the default.  */
> > +  NON_PREFIXED_D,		/* All 16-bits are valid.  */
> > +  NON_PREFIXED_DS,		/* Bottom 2 bits must be 0.  */
> > +  NON_PREFIXED_DQ,		/* Bottom 4 bits must be 0.  */
> > +  NON_PREFIXED_X		/* No offset memory form exists.  */
> > +};
> 
> Yeah the DS- and DQ-form descriptions here are nicer I think, thanks.
> 
> Maybe non_prefixed_form is a clearer name?  But it is longer of course.
> You decide.

That is fine.

> 
> > +      if (SYMBOL_REF_P (x) && !SYMBOL_REF_LOCAL_P (x))
> > +	fputs ("@got", file);
> > +
> >        fputs ("@pcrel", file);
> 
> I'd just use fprintf btw, GCC knows since decades to optimise that to
> fputs, and it is easier to read IMO.  Not that fputs is so super bad,
> but every little we do not have to think helps (helps us think, think
> about more important matters!)

Ok.  It is just the many years of doing C code before GCC did the optimization
is ingrained on me.
 
> > +/* Given an address (ADDR), a mode (MODE), and what the format of the
> > +   non-prefixed address (NON_PREFIXED_INSN) is, return the instruction format
> > +   for the address.  */
> > +
> > +enum insn_form
> > +address_to_insn_form (rtx addr,
> > +		      machine_mode mode,
> > +		      enum non_prefixed non_prefixed_insn)
> 
> non_prefixed_form, instead?
> 
> 
> > +{
> > +  rtx op0, op1;
> 
> You can declare these at first use.  Declaring things in multiple blocks
> (so with shorter scopes) is a bit nicer.
> 
> > +  /* If we don't support offset addressing, make sure only indexed addressing
> > +     is allowed.  We special case SDmode so that the register allocator does
> > +     try to move SDmode through GPR registers, but instead uses the 32-bit
> > +     integer read/write instructions for the floating point registers.  */
> 
> Does *not* try?
> 
> Read/write, do you mean load/store?  lfiwzx and friends?
> 
> 
> > +  if (GET_CODE (addr) != PLUS)
> > +    return GET_CODE (addr) == LO_SUM ? INSN_FORM_LO_SUM : INSN_FORM_BAD;
> 
>   if (GET_CODE (addr) == LO_SUM)
>     return INSN_FORM_LO_SUM;
> 
>   if (GET_CODE (addr) != PLUS)
>     return INSN_FORM_BAD;
> 
> (Avoid using the conditional operator if you can use an "if" statement
> just as well; easier to read).
> 
> > +  op0 = XEXP (addr, 0);
> > +  op1 = XEXP (addr, 1);
> > +
> > +  if (REG_P (op1) || SUBREG_P (op1))
> > +    return INSN_FORM_X;
> 
> I think you should have checked op0 here as well?

Yes probably.  Too many years of writing secondary reload patterns where the
first argument can be funny during the reload phase. :-)

> > +  /* If it isn't pc-relative, check for 16-bit D/DS/DQ-form.  */
> > +  if (!REG_P (op0) && !SUBREG_P (op0))
> > +    return INSN_FORM_BAD;
> 
> (Instead of only later here).
> 
> Overall this looks like it will work nicely, thanks!

Did you want the patch updated now, or should I wait for you to comment on the
next set of patches?