This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH V2 2/8] bpf: new GCC port
On Mon, Aug 19, 2019 at 08:57:22PM +0100, Richard Sandiford wrote:
> > +/*** Order of Allocation of Registers. */
> > +
> > +/* We generally want to put call-clobbered registers ahead of
> > + call-saved ones. (IRA expects this.) */
> > +#define REG_ALLOC_ORDER \
> > + {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11}
>
> Do you gain much by defining this? I would have expected better
> code without, given the architecture is so regular.
It does exactly the same as not defining REG_ALLOC_ORDER, just a tiny
bit less efficient.
> > +#define REG_CLASS_CONTENTS \
> > +{ \
> > + 0x00000000, /* NO_REGS */ \
> > + 0x000007ff, /* GR_REGS */ \
> > + 0x000007ff, /* ALL_REGS */ \
> > +}
> > +
> > +/* A C expression whose value is a register class containing hard
> > + register REGNO. In general there is more that one such class;
> > + choose a class which is "minimal", meaning that no smaller class
> > + also contains the register. */
> > +#define REGNO_REG_CLASS(REGNO) ((REGNO) < 11 ? GR_REGS : ALL_REGS)
>
> Did you mean to include register 11 in ALL_REGS in REG_CLASS_CONTENTS?
> If not, then there doesn't seem to be any distinction between ALL_REGS
> and GR_REGS, and it'd be better to make one the alias of the other
> (and make REGNO_REG_CLASS return NO_REGS for 11).
ALL_REGS is required to contain all (hard) registers, too. I wonder what
will go wrong this way... Well nothing too obvious, apparently! :-)
> > +(define_insn "*mulsi3_extended"
> > + [(set (match_operand:DI 0 "register_operand" "=r,r")
> > + (sign_extend:DI
> > + (mult:SI (match_operand:SI 1 "register_operand" "0,0")
> > + (match_operand:SI 2 "reg_or_imm_operand" "r,I"))))]
> > + ""
> > + "mul32\t%0,%2"
> > + [(set_attr "type" "alu32")])
>
> There's a named pattern for this: mulsidi3. You might get better
> code by using that name instead.
mulsidi3 is something else (it extends the operands before the mult).
> > +(define_expand "extendsidi2"
> > + [(set (match_operand:DI 0 "register_operand" "=r")
> > + (sign_extend:DI (match_operand:SI 1 "register_operand" "r")))]
>
> define_expands shouldn't have constraints. (Same for the rest of the file.)
>
> > [...]
> > +(define_expand "mov<AMM:mode>"
> > + [(set (match_operand:AMM 0 "general_operand" "")
> > + (match_operand:AMM 1 "general_operand" ""))]
Not empty constraints, either... That is, you do not need to write them,
and internally it will be the same thing.
Segher