Re: [PATCH] MIPS32 DSP intrinsics

[Richard Sandiford <rsandifo at redhat dot com>]

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