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
