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?
--
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meiss...@linux.ibm.com, phone: +1 (978) 899-4797
