Re: [PATCH] MIPS32 DSP intrinsics

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

Re: [PATCH] MIPS32 DSP intrinsicsHello,

  I combined 17 immediate and variable versions of
intrinsics.  And I used macro to reduce several
instructions.  Please see the attached files.

  Here is the list of 17 intrinsics that can accept
immediate numbers and variables as parameters.

v4i8 __builtin_mips_shll_qb (v4i8, imm0_7);
v2q15 __builtin_mips_shll_ph (v2q15, imm0_15);
v2q15 __builtin_mips_shll_s_ph (v2q15, imm0_15);
q31 __builtin_mips_shll_s_w (q31, imm0_31);
v4i8 __builtin_mips_shrl_qb (v4i8, imm0_7);
v2q15 __builtin_mips_shra_ph (v2q15, imm0_15);
v2q15 __builtin_mips_shra_r_ph (v2q15, imm0_15);
q31 __builtin_mips_shra_r_w (q31, imm0_31);
v4i8 __builtin_mips_repl_qb (imm0_255);
v2q15 __builtin_mips_repl_ph (imm_n512_511);
i32 __builtin_mips_extr_w (a64, imm0_31);
i32 __builtin_mips_extr_r_w (a64, imm0_31);
i32 __builtin_mips_extr_rs_w (a64, imm0_31);
i32 __builtin_mips_extr_s_h (a64, imm0_31);
i32 __builtin_mips_extp (a64, imm0_31);
i32 __builtin_mips_extpdp (a64, imm0_31);
a64 __builtin_mips_shilo (a64, imm_n32_31);

  The range of immediate numbers and the range
of variables are the same for all intrinsics
except __builtin_mips_repl_ph() which immediate
version accepts -512 to 511 (repl.ph), but variable 
version can process -32768 to 32767 (replv.ph).  
Thus, I created a predicate "reg_imm10_operand" for 
the "repl.ph" instruction pattern, but used the predicate 
"arith_operand" for other 16 instruction patterns.
For repl.ph, GCC will copy numbers to a register and pick the
variable instructions, if numbers are not in the range
(-512 to 511).
For other 16 instruction patterns, there is no benefit by
moving numbers to a register, because both immediate and
variable versions accept same ranges.

  One small problem is that when users use immediate
numbers out of the range, GCC doesn't give warnings but
just silently masks numbers to the range upon emitting
instructions.  Is this behavior ok?  If not, I may need to
add new types of "mips_builtin_type" for checking immediate
numbers, and I may give wanrings during expanding builtins
at "mips_expand_builtin_direct()".  Any suggestion?  
Thanks a lot!

Regards,
Chao-ying

----- Original Message ----- 
From: Richard Sandiford 
To: Fu, Chao-Ying 
Cc: Thekkath, Radhika ; 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

Attachment: mips-dsp.md
Description: Binary data

Attachment: gcc06212005.diff
Description: Binary data