This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] MIPS32 DSP intrinsics


  Previously, I didn't notice that from the MIPS ISA spec.
mtlo/mthi will destroy hi/lo if they are moved between
{mult, ...} and mfhi/mflo.  The same restriction applies
to $ac1, $ac2, $ac3.  So, I tried to reuse this pattern:
"*mfhilo_<mode>".  Then, I need to create six register classes
for ac1hi, ac1lo, ac2hi, ac2lo, ac3hi, ac3lo.  As you pointed out,
my implementation is not correct.
I will try to allocate 6 unused single characters for 6 new classes.
Or, are there other ways to deal with this restriction?

NOTE: It seems that GCC3.4 doesn't implement this restriction
of mtlo/mthi in "mips.md".

Regards,
Chao-ying


----- Original Message ----- 
From: "Richard Sandiford" <richard@codesourcery.com>
To: "Chao-ying Fu" <fu@mips.com>
Cc: "Thekkath, Radhika" <radhika@mercury.mips.com>;
<gcc-patches@gcc.gnu.org>
Sent: Friday, July 08, 2005 12:29 AM
Subject: Re: [PATCH] MIPS32 DSP intrinsics


> 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
>


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]