Re: [PATCH] MIPS32 DSP intrinsics

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

  Thanks for your review!  Some feedbacks are as follows.

1. Yes, I did a normal simulator-based dejagnu regression test.
Test Run By fu on Tue Jul 19 13:50:11 2005
Target is mipsisa32-unknown-elf
Host   is i686-pc-linux-gnu

2.
>> + #define CODE_FOR_mips_addq_s_ph CODE_FOR_mips_addsv2hi3
>> + #define CODE_FOR_mips_addq_s_w CODE_FOR_mips_addssi3

> ...stuff like this shouldn't be necessary.  Just give the .md pattern
> the name you want.

  Because I use macro in "mips-dsp.md", some auto-generated instruction
names are not what I want.  Thus, I need to have these defines.

3.
>> + mdsp
>> + Target Report RejectNegative Mask(DSP)
>> + Use MIPS-DSP instructions

> Why RejectNegative?

  There is no use for "-mno-dsp" currently, so I use "RejectNegative".
But, I can remove "RejectNegative" to create "-mno-dsp", if this is better.

  I will work on the patch again.  Thanks!

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: Saturday, July 16, 2005 1:05 AM
Subject: Re: [PATCH] MIPS32 DSP intrinsics


> "Chao-ying Fu" <fu@mips.com> writes:
> > 4. Add two tests: mips32-dsp.c mips32-dsp-type.c for scanning
> > assembly instructions.  (I also tested by executing them using our
> > simulator.)
> >
> > # make RUNTEST=/home/dejagnu/runtest check-gcc
> > RUNTESTFLAGS="--target_board=mips-sim  mips.exp=mips32-dsp*"
> >
> >                 === gcc Summary ===
> >
> > # of expected passes            97
> > /home/sweng_scratrch1/gcc-mainline/builddsp/gcc/xgcc  version 4.1.0
20050712
> > (experimental)
> >
> >   I didn't bootstrap GCC, but just checked the warning message while
> > building GCC..
>
> Did you also do a normal simulator-based dejagnu regression test?
> If so, on which target?  Again, always give this information when
> posting a patch.
>
> > !    - 6 DSP control register  */
>
> s/register/registers/
>
> > --- 7356,7362 ----
> >        in LO and HI, the high word always comes first.  We therefore
> >        can't allow values stored in HI to change between single-word
> >        and multi-word modes.  */
> > !   if (reg_classes_intersect_p (ACC_REGS, class))
> >       return true;
>
> You need to change the comment too.
>
> > --- 7438,7444 ----
> >
> >     /* Copying from HI or LO to anywhere other than a general register
> >        requires a general register.  */
> > !   if (reg_class_subset_p (class, ACC_REGS))
>
> Same here.
>
> >   mips_vector_mode_supported_p (enum machine_mode mode)
> >   {
> > !   if ((mode == V2SFmode && TARGET_PAIRED_SINGLE_FLOAT) ||
> > !       (TARGET_DSP && (mode == V2HImode || mode == V4QImode)))
> >       return true;
> >     else
> >       return false;
>
> According to the coding standards, || should be on the second line.
> And while I'm no great fan of the "if (..) return true; else return
false;"
> construct, if we're using it, we might as well put the new test in its
> own else-if clause.
>
> > + #define CODE_FOR_mips_addq_ph CODE_FOR_addv2hi3
>
> This sort of #define is fine.  It's making the builtin an alias for
> a well-known rtl operation.  But...
>
> > + #define CODE_FOR_mips_addq_s_ph CODE_FOR_mips_addsv2hi3
> > + #define CODE_FOR_mips_addq_s_w CODE_FOR_mips_addssi3
>
> ...stuff like this shouldn't be necessary.  Just give the .md pattern
> the name you want.
>
> > + /* Define __builtin_mips_bposge<value>.  <value> is 32 for the MIPS32
DSP branch
> > +    instruction.  */
> > + #define BPOSGE_BUILTIN(INSN, BUILTIN_TYPE, FUNCTION_TYPE,
TARGET_FLAGS) \
> > +   { CODE_FOR_mips_ ## INSN, 0, "__builtin_mips_" #INSN, \
> > +     MIPS_BUILTIN_ ## BUILTIN_TYPE, FUNCTION_TYPE, TARGET_FLAGS }
>
> I realise that you've probably written it this way to accomodate the
MIPS64
> support, but the comment's a bit confusing.  It mentions <value>, but
> nothing called "value" appears in the macro definition.
>
> Also, make lines shorter than 80 characters.  (First line is 80.)
>
> > !   error ("Invalid argument");
>
> Error messages don't begin with caps.  "invalid argument to builtin
> function" might be more descriptive.
>
> >   static rtx
> > ! mips_expand_builtin_direct (enum insn_code icode, rtx target, tree
arglist,
> > !     int has_target)
>
> The new parameter should be a "bool".  Thanks for reusing the function
> though.
>
> > !   if (has_target)
> >       {
> > !       target = mips_prepare_builtin_target (icode, 0, target);
> > !       /* We need to test if arglist is not zero.  Some instructions
have extra
> > ! clobber registers.  */
> > !       for (i = 1; i < insn_data[icode].n_operands && arglist != 0;
i++)
> > ! ops[i] = mips_prepare_builtin_arg (icode, i, &arglist);
> >
> > !       switch (i)
> > ! {
> > ! case 2:
> > !   emit_insn (GEN_FCN (icode) (target, ops[1]));
> > !   break;
> >
> > ! case 3:
> > !   emit_insn (GEN_FCN (icode) (target, ops[1], ops[2]));
> > !   break;
> >
> > ! case 4:
> > !   emit_insn (GEN_FCN (icode) (target, ops[1], ops[2], ops[3]));
> > !   break;
> > !
> > ! default:
> > !   gcc_unreachable ();
> > ! }
> > !     }
> > !   else /* no target */
> > !     {
> > !       for (i = 0; i < insn_data[icode].n_operands && arglist != 0;
i++)
> > ! ops[i] = mips_prepare_builtin_arg (icode, i, &arglist);
> > !
> > !       switch (i)
> > ! {
> > ! case 2:
> > !   emit_insn (GEN_FCN (icode) (ops[0], ops[1]));
> > !   break;
> > !
> > ! default:
> > !   gcc_unreachable ();
> > ! }
>
> If you put target in ops[0] you can avoid the duplicated "for" and
> "switch" statements.
>
> > + /* Expand a bposge builtin of type BUILTIN_TYPE.
> > +    TARGET, if nonnull, suggests a good place to put the boolean
result.
> > +    Ex: ("bposge32" is a branch instruction that jumps to the
destination, when
> > + the "pos" control register is greater than or equal to 32 in MIPS32
> > + DSP ASE.)
> > + li target, 0
> > + bposge32 label1
> > + j label2
> > +    label1:
> > + li target, 1
> > +    label2:
> > + ...  */
>
> /* Expand a bposge builtin of type BUILTIN_TYPE.  TARGET, if nonnull,
>    suggests a good place to put the boolean result.
>
>    The sequence we want is:
>
> li target, 0
> bposge* label1
> j label2
>    label1:
> li target, 1
>    label2:  */
>
> > + mdsp
> > + Target Report RejectNegative Mask(DSP)
> > + Use MIPS-DSP instructions
>
> Why RejectNegative?
>
> > --- 569,575 ----
> >   -mips16  -mno-mips16  -mabi=@var{abi}  -mabicalls  -mno-abicalls @gol
> >   -mxgot  -mno-xgot  -mgp32  -mgp64  -mfp32  -mfp64 @gol
> >   -mhard-float  -msoft-float  -msingle-float  -mdouble-float @gol
> > ! -mdsp -mpaired-single  -mips3d @gol
> >   -mlong64  -mlong32  -msym32  -mno-sym32 @gol
> >   -G@var{num}  -membedded-data  -mno-embedded-data @gol
> >   -muninit-const-in-rodata  -mno-uninit-const-in-rodata @gol
>
> Two spaces after -mdsp, as for all the other options.
>
> The docs are generally good, but at times they give the impression
> of being written as a stand-alone document, not as part of the overall
> gcc manual.  Can you try to structure it a bit more like the existing
> paired-single documentation?
>
> The biggest problem is that you're talking about Q31 data before
> any such support exists.  I assume that's part of the forthcoming
> MIPS64 patch?  Please make sure that you're documenting exactly
> what you're adding.
>
> The patch looks pretty good otherwise, thanks.
>
> Richard
>