Re: [PATCH] MIPS32 DSP intrinsics

[Richard Sandiford <richard at codesourcery dot com>]

Further to the reply about testing, if you have a big patch, and make
some changes to it, it really helps if you point out what those changes
are and say why you decided to write the code like that.  (I realise you
did this in the previous patch, and thanks again for that.)

In this case you simply said:

> It includes the changes that you pointed out and GCC
> document updates (invoke.texi and extend.texi).

but I notice the following new code:

"Chao-ying Fu" <fu@mips.com> writes:
> + #define AC1HI_REGNUM	(DSP_ACC_REG_FIRST + 0)
> + #define AC1LO_REGNUM	(DSP_ACC_REG_FIRST + 1)
> + #define AC2HI_REGNUM	(DSP_ACC_REG_FIRST + 2)
> + #define AC2LO_REGNUM	(DSP_ACC_REG_FIRST + 3)
> + #define AC3HI_REGNUM	(DSP_ACC_REG_FIRST + 4)
> + #define AC3LO_REGNUM	(DSP_ACC_REG_FIRST + 5)

> !    `YG' is for 0 valued vector constants.
> !    `YA' is for unsigned 6-bit constants.
> !    `YB' is for signed 10-bit constants.
> !    `Ya' is AC1HI of DSP accumulators.
> !    `Yb' is AC1LO of DSP accumulators.
> !    `Yc' is AC2HI of DSP accumulators.
> !    `Yd' is AC2LO of DSP accumulators.
> !    `Ye' is AC3HI of DSP accumulators.
> !    `Yf' is AC3LO of DSP accumulators.  */
>   
>   #define EXTRA_CONSTRAINT_Y(OP,STR)					\
>      [...]
> +    : ((STR)[1] == 'a')	  ? (REG_P (OP) && REGNO (OP) == AC1HI_REGNUM)	\
> +    : ((STR)[1] == 'b')	  ? (REG_P (OP) && REGNO (OP) == AC1LO_REGNUM)	\
> +    : ((STR)[1] == 'c')	  ? (REG_P (OP) && REGNO (OP) == AC2HI_REGNUM)	\
> +    : ((STR)[1] == 'd')	  ? (REG_P (OP) && REGNO (OP) == AC2LO_REGNUM)	\
> +    : ((STR)[1] == 'e')	  ? (REG_P (OP) && REGNO (OP) == AC3HI_REGNUM)	\
> +    : ((STR)[1] == 'f')	  ? (REG_P (OP) && REGNO (OP) == AC3LO_REGNUM)	\

This just isn't right.  Register constraints are based on register classes,
not rtl checks, and should be provided by REG_CLASS_FROM_{LETTER,CONSTRAINT}.

In the last review I said:

---------------------------------------------------------------------------
GR_REGS <- MD_REGS is not an alternative we can allow.
I said in the last review that there was no need for a separate
register class for ACC_REGS - MD_REGS.  Unfortunately, this move
problem shows that I was wrong.  The move patterns should allow
GR_REGS <- (ACC_REGS - MD_REGS) but not GR_REGS <- MD_REGS.
---------------------------------------------------------------------------

So what I meant was that the move patterns should use a single register
class for (ACC_REGS - MD_REGS).  In other words, you'd have three classes:

   MD_REGS       ac0
   DSP_ACC_REGS  ac1-ac3
   ACC_REGS      ac0-ac3

You could then allow GR_REGS <- DSP_ACC_REGS in the move patterns,
excluding the problematic GR_REGS <- MD_REGS moves.

However, I notice that your new patch instead extends the mfhilo
patterns so that they're used for the new accumulator registers.
Why did you do that?  Is it because the same restrictions apply
to the new registers?  (This is why reviewing a patch without
h/w docs is a bit difficult ;)

If so, you should at least adjust the comments above mfhilo to
say that.  As it stands, you have a pattern that applies to all
DSP registers but a comment that just talks about HI and LO.

Richard