Re: [PATCH] MIPS32 DSP intrinsics

["Chao-ying Fu" <fu at mips dot com>]

Hello,

  Thanks for your comments! I will merge two builtin

functions (immediate and variable versions) into one.

This can reduce 19 builtin functions overall.

It will take some time to change the code. Other items

from your comments were fixed.

  About the testing, we modified binutils to assemble DSP

instructions, and used our own simulator to test the program.

The GNU simulator has not been changed to support DSP

instructions. This needs to be done sometime.

Regards,
Chao-ying


----- Original Message ----- 
From: "Richard Sandiford" <rsandifo@redhat.com>
To: "Chao-ying Fu" <fu@mips.com>
Cc: "Thekkath, Radhika" <radhika@mercury.mips.com>;
<gcc-patches@gcc.gnu.org>
Sent: Wednesday, May 25, 2005 11:33 PM
Subject: Re: [PATCH] MIPS32 DSP intrinsics


> Thanks for your message.  To be honest, it didn't really provide
> many of details I was after, but it certainly did help a bit.
>
> "Chao-ying Fu" <fu@mips.com> writes:
> >   I didn't try to use macro to minimize the machine description file
> > (mips-dsp.md).
>
> Please do!  There seems to be a lot of scope for a V2HI/V4QI macro,
> and perhaps others.
>
> > (Actually, "mips-dsp.md" was not generated by hand, but by a program
> > which parses a table.
>
> Yeah, I'm afraid it shows ;)  Did you consider contributing that
> generator program and its input file?  Could the same generator
> be used for other MIPS builtins too?
>
> I realise that it can be useful to start with automatically-generated
> output, but if the generator program isn't contributed, we'll have to
> treat the new .md file in the same way as any other.
>
> > Most of the dsp instructions use UNSPEC_*, except for modulo add,
> > modulo sub, indexed load instructions, and branch.  Because many DSP
> > instructions operate on fixed-point data with optional saturation and
> > rounding, the current RTL operators cannot support them directly.  I
> > am not sure if it makes sense to code these DSP instructions using
> > existing RTL operators.  (It will be nice if we can create a new set
> > of RTL operators for fixed-point arithmetic in the future.  Ex:
> > saturating operators, rounding operators.)
>
> Agreed.  It's definitely OK to use UNSPECs for things that have
> no direct rtl equivalent.
>
> >   The usage guide for DSP intrinsics is attached.  Thanks a lot!
>
> I think you misunderstood my point about documentation.  We need
> a patch to the gcc manual to document the new feature.  For example,
> the new options should be documented in doc/invoke.texi and the new
> builtins should be documented in doc/extend.texi.
>
> Anyway, as far as the patch itself goes, I guess I have to dive in at
> the deep end to some extent.  Here are some initial comments:
>
>   - Why did you decide to have two separate builtins for instructions
>     like shrl.qb and shrlv.qb?  Wouldn't it be more user-friendly to
>     have a single builtin that selects the right insn automatically?
>     GCC is more than capable of doing that, and I think it would
>     actually make the patch a lot simpler.
>
>   - Related: if you're going to have patterns that require CONST_INTs
>     in a certain range, you should add new predicates for them.
>     As well as being more correct in theory, it would avoid
>     hackery like:
>
> !   /* We need to test if the operand must be an immediate.  */
> !   if (insn_data[icode].operand[op].predicate == immediate_operand)
> !     {
> !       if (TREE_CODE (arg) != INTEGER_CST)
>
>     It would also cut down on the number of MIPS_BUILTIN_DIRECT_IMM*s.
>
>   - You're checking !TARGET_MIPS16 every time you check TARGET_DSP.
>     It would be better to either (a) treat the combination of
>     -mdsp and -mips16 as an error or (b) turn -mdsp off if -mips16
>     is passed.  You can do this in override_options.
>
>   - You should mark the new accumulator registers as fixed if !TARGET_DSP.
>     See the way ST_REGs are handled in mips_conditional_register_usage.
>     If you don't, there's always the risk that the register allocator
>     might use them for -mno-dsp.
>
>   - The new DSP_ACC_REGS class is not independently useful.
>     Only MD_AND_DSP_ACC_REGS is.  You might as well just add
>     the latter and give it a shorter name like ACC_REGS.
>
>   - As I said above, CONDITONAL_REGISTER_USAGE should hide the DSP
>     accumulators from the register allocator when -mdsp is not in use.
>     You can therefore map 'a' to ACC_REGS unconditionally.
>
>   - You should generalise existing hi/lo-checks where applicable,
>     rather than treat {MD_AND_DSP_,DSP_,}ACC_REGS as a separate case.
>
>   - You have two sets of constants for the control registers.
>     One is enough.  If you put the define_constants in mips.md,
>     you can use them in mips.c too.
>
>   - Please don't add more uses of the "register" keyword.  The fact
>     that you probably cut-&-pasted it from somewhere is no excuse. ;)
>
> That's just a first cut, really.  I'm afraid it might take a
> couple more patch/review iterations before the changes go in,
> but that isn't unusual for a patch of this size.
>
> Oh, and when you post a patch, please remember to say how you
> tested it.
>
> Thanks,
> Richard
>