[MIPS] Patch for DSP REV 2 builtin functions

Fu, Chao-Ying fu@mips.com
Thu Mar 8 00:52:00 GMT 2007


Hi,

  Thanks for your review.  I think I fix all issues.

Regards,
Chao-ying

> >   We would like to contribute our patch for MIPS DSP
> > ASE REV 2 builtin functions.  The binutils and simulator
> > supports for new instructions are already checked in to FSF.
> > We just wonder if the GCC mainline can accept new features in the 
> > currnet stage.
> 
> I'm showing my ignorance here, but is it the case that all revision 1
> MIPS parts that implement the DSP ASE implement revision 1 of the ASE,
> and that all revision 2 MIPS parts that implement the ASE implement
> revision 2 of the ASE?  Or isn't it that simple?

  The REV 2 adds more instructions, and requires the original DSP ASE
architecture (REV 1).  All revision 2 MIPS parts implement both rev 1 and
rev 2.

> 
> Anyway, the patch looks good to me, thanks.  The obvious missing piece
> is the documentation; the patch can't really go in without 

  Yes, I updated invoke.texi and extend.texi to describe the option (-mdspr2)
and the DSP REV 2 builtin functions.

> it.  I've some
> other comments too, varying from very minor to more significant:
> 
> > +	  types[MIPS_SI_FTYPE_SI_SI_SI]
> > +	    = build_function_type_list (intSI_type_node,
> > +					intSI_type_node, 
> intSI_type_node, intSI_type_node,
> > +					NULL_TREE);
> > +
> > +	  types[MIPS_DI_FTYPE_DI_USI_USI]
> > +	    = build_function_type_list (intDI_type_node,
> > +					intDI_type_node, 
> unsigned_intSI_type_node, unsigned_intSI_type_node,
> > +					NULL_TREE);
> > +
> > +	  types[MIPS_DI_FTYPE_SI_SI]
> > +	    = build_function_type_list (intDI_type_node,
> > +					intSI_type_node, 
> intSI_type_node,
> > +					NULL_TREE);
> > +
> > +	  types[MIPS_DI_FTYPE_USI_USI]
> > +	    = build_function_type_list (intDI_type_node,
> > +					
> unsigned_intSI_type_node, unsigned_intSI_type_node,
> > +					NULL_TREE);
> > +
> > +	  types[MIPS_V2HI_FTYPE_SI_SI_SI]
> > +	    = build_function_type_list (V2HI_type_node,
> > +					intSI_type_node, 
> intSI_type_node, intSI_type_node,
> > +					NULL_TREE);
> > +
> > +	}
> 
> Watch the long lines.

  Yes.  Fixed.
> 
> > (define_insn "mips_append"
> >   [(set (match_operand:SI 0 "register_operand" "=d")
> > 	(unspec:SI [(match_operand:SI 1 "register_operand" "0")
> > 		    (match_operand:SI 2 "reg_or_0_operand" "dJ")
> > 		    (match_operand:SI 3 "immediate_operand" "i")]
> 
> Use "const_int_operand" "n" rather than "immediate_operand" "I" since
> your code really does expect a CONST_INT here.  Likewise for the other
> patterns.  (Using "immediate_operand" would in principle 
> allow symbolic
> constants too.)

  Yes.  Fixed.

> 
> > (define_insn "mips_dpa_w_ph"
> >   [(set (match_operand:DI 0 "register_operand" "=a")
> > 	(unspec:DI [(match_operand:DI 1 "register_operand" "0")
> > 		    (match_operand:V2HI 2 "reg_or_0_operand" "dYG")
> > 		    (match_operand:V2HI 3 "reg_or_0_operand" "dYG")]
> > 		   UNSPEC_DPA_W_PH))]
> >   "TARGET_DSPR2 && !TARGET_64BIT"
> >   "dpa.w.ph\t%q0,%z2,%z3"
> >   [(set_attr "type"	"imadd")
> >    (set_attr "mode"	"SI")])
> 
> What happens if you try to use the built-in functions on 
> 64-bit targets?
> Do we ICE?  Same for other such patterns.

  Yes for the previous implementation.

> 
> I suspect you need a separate table of built-in functions for the
> 32-bit-only built-in functions.  It's OK for those functions not to
> be recognised at all if TARGET_64BIT.

  Ok.  We need one more field in "struct bdesc_map" to identify
unsupported target flags.  And, we need to separate the DSP REV 1
table as well, because some builtins are not for 64-bit targets, either.
I merged the REV 2 table to the original dsp_bdesc, and created
a new table for the 32-bit only builtins.
> 
> > (define_insn "mips_madd"
> >   [(set (match_operand:DI 0 "register_operand" "=a")
> > 	(plus:DI
> > 	 (mult:DI (sign_extend:DI
> > 		   (match_operand:SI 2 "reg_or_0_operand" "dJ"))
> > 		  (sign_extend:DI
> > 		   (match_operand:SI 3 "reg_or_0_operand" "dJ")))
> > 	 (match_operand:DI 1 "register_operand" "0")))]
> >   "TARGET_DSPR2 && !TARGET_64BIT"
> >   "madd\t%q0,%z2,%z3"
> >   [(set_attr "type"	"imadd")
> >    (set_attr "mode"	"SI")])
> 
> These operands must be "register_operand" "d".  It isn't valid to
> have a sign_extend or zero_extend of a const_int.  Likewise for
> other operands inside an extension.

  Yes.  Fixed.

> 
> > (define_insn "mulv2hi3"
> >   [(parallel
> >     [(set (match_operand:V2HI 0 "register_operand" "=d")
> > 	  (mult:V2HI (match_operand:V2HI 1 "reg_or_0_operand" "dYG")
> > 		     (match_operand:V2HI 2 "reg_or_0_operand" "dYG")))
> >      (set (reg:CCDSP CCDSP_OU_REGNUM)
> > 	  (unspec:CCDSP [(match_dup 1) (match_dup 2)] UNSPEC_MUL_PH))
> >      (clobber (match_scratch:DI 3 "=x"))])]
> >   "TARGET_DSPR2"
> >   "mul.ph\t%0,%z1,%z2"
> >   [(set_attr "type"	"imul3")
> >    (set_attr "mode"	"SI")])
> 
> I think this too should be "register_operand" "d".  If multiplications
> against zero don't get optimised to zero, well, they should. ;)
> And a constant in operand 1 isn't canonical rtl.

  Yes.  Fixed.

> 
> > /* Test MIPS32 DSP REV 2 MULT instruction */
> > /* { dg-do compile } */
> > /* { dg-mips-options "-march=mips32r2 -mdspr2 -O2 
> -ffixed-hi -ffixed-lo" } */
> > /* { dg-final { scan-assembler "mult" } } */
> 
> Better make this "\tmult\t" so that you know you're not getting
> unsigned multiplication by accident.  Same applies to other
> scan-assemblers for a particular insn.

  Yes.  Fixed.

> 
> > /* Test MIPS32 DSP REV 2 instructions */
> > /* { dg-do run { target mipsisa32r2*-*-* } } */
> > /* { dg-mips-options "-march=mips32r2 -mdspr2 -O2" } */
> 
> I know this is only a test, but since it's almost GNU format, 
> I'll nitpick:
> 
> > void test_MIPS_DSPR2();
> 
> ...space before "(", and make it "(void)".
> 
> >   if(r != s)
> 
> space before "(" here, and in other tests.

  Yes.  Fixed.

> 
> Looks good otherwise, thanks.  I'd like to see the revised patch
> before giving an OK though.
> 
> Richard
> 

gcc/ChangeLog
2007-03-07  Chao-ying Fu  <fu@mips.com>

	* doc/extend.texi (MIPS DSP Built-in Functions): Document the DSP REV 2.
	* doc/invoke.texi (-mdspr2): Document new option.
	* config/mips/mips.md (UNSPEC_ABSQ_S_QB .. UNSPEC_DPSQX_SA_W_PH):
	New unspec for DSP REV 2.
	(<u>mulsidi3_32bit_internal): Check if !TARGET_DSPR2, because 
	these instructions are extended in DSP REV 2.
	(mips-dspr2.md): Include.
	* config/mips/mips.opt (mdspr2): New option.
	* config/mips/mips.c (mips_function_type): Add MIPS_V4QI_FTYPE_V4QI,
	MIPS_SI_FTYPE_SI_SI_SI, MIPS_DI_FTYPE_DI_USI_USI, MIPS_DI_FTYPE_SI_SI,
	MIPS_DI_FTYPE_USI_USI, MIPS_V2HI_FTYPE_SI_SI_SI.
	(override_options): Check TARGET_DSPR2 to enable MASK_DSP.
	(CODE_FOR_mips_mul_ph): Define it to CODE_FOR_mulv2hi3.
	(dsp_bdesc): Add DSP REV 2 builtins.  Remove 32-bit only DSP builtins.
	(dsp_32only_bdesc): New description table for 32-bit only DSP REV 1 and 2 builtins.
	(bdesc_map): Add one field of unsupported_target_flags.
	(bdesc_arrays):  Update entries to have extra fields.  Add dsp_32only_bdesc.
	(mips_init_builtins): Initialize new function types.
	Check unsupported_target_fileds to filter out builtins.
	* config/mips/mips.h (TARGET_CPU_CPP_BUILTINS): Define __mips_dspr2 if
	TARGET_DSPR2.
	(ASM_SPEC): Pass mdspr2 to the assembler.
	* config/mips/mips32-dspr2.md: New file.

gcc/testsuite/ChangeLog
2007-03-07  Chao-ying Fu  <fu@mips.com>

	* gcc.target/mips/mips32-dspr2-type.c: New test.
	* gcc.target/mips/mips32-dspr2.c: New test.
	* gcc.target/mips/dspr2-MULT.c: New test.
	* gcc.target/mips/dspr2-MULTU.c: New test.
	* gcc.target/mips/mips32-dsp-run.c: New test to check execution.
	* gcc.target/mips/mips32-dsp.c: Change v4i8 typedef to use signed char.
	Adjust some formats.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: gcc.diff
Type: application/octet-stream
Size: 41039 bytes
Desc: gcc.diff
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20070308/10711f7e/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mips-dspr2.md
Type: application/octet-stream
Size: 20898 bytes
Desc: mips-dspr2.md
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20070308/10711f7e/attachment-0001.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: dspr2-MULT.c
Type: application/octet-stream
Size: 510 bytes
Desc: dspr2-MULT.c
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20070308/10711f7e/attachment-0002.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: dspr2-MULTU.c
Type: application/octet-stream
Size: 521 bytes
Desc: dspr2-MULTU.c
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20070308/10711f7e/attachment-0003.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mips32-dspr2.c
Type: application/octet-stream
Size: 13296 bytes
Desc: mips32-dspr2.c
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20070308/10711f7e/attachment-0004.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mips32-dspr2-type.c
Type: application/octet-stream
Size: 275 bytes
Desc: mips32-dspr2-type.c
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20070308/10711f7e/attachment-0005.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mips32-dsp-run.c
Type: application/octet-stream
Size: 24048 bytes
Desc: mips32-dsp-run.c
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20070308/10711f7e/attachment-0006.obj>


More information about the Gcc-patches mailing list