diff mbox

[MIPS] fix mips_prepend insn.

Message ID 87liolvzby.fsf@firetop.home
State New
Headers show

Commit Message

Richard Sandiford Feb. 2, 2012, 6:56 p.m. UTC 
Liu <proljc@gmail.com> writes:
> diff --git a/gcc/config/mips/mips-dspr2.md b/gcc/config/mips/mips-dspr2.md
> index 5ae902f..6853b9d 100644
> --- a/gcc/config/mips/mips-dspr2.md
> +++ b/gcc/config/mips/mips-dspr2.md
> @@ -345,7 +345,7 @@
>     (set_attr "mode"	"SI")])
>  
>  (define_insn "mips_prepend"
> -  [(set (match_operand:SI 0 "register_operand" "=d")
> +  [(set (match_operand:DI 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 "const_int_operand" "n")]

This pattern maps directly to __builtin_mips_prepend, which is defined
to take and return an SI type, so :SI is the correct choice here.

I agree it might be nice to have a function that operates on 64-bit
values for 64-bit targets though.  For compatibility reasons,
we'd probably have to define both a new function and a new pattern
(or at least an iterator version of the current pattern).
There's currently no way of generating PREPENDD or PREPENDW either.

I consider this API to be owned by MTI, so we'd need to coordinate
with them.  Chao-Ying, what do you think?  Do you already have
something like this internally?

> @@ -353,7 +353,7 @@
>    "ISA_HAS_DSPR2"
>  {
>    if (INTVAL (operands[3]) & ~(unsigned HOST_WIDE_INT) 31)
> -    operands[2] = GEN_INT (INTVAL (operands[2]) & 31);
> +    operands[3] = GEN_INT (INTVAL (operands[3]) & 31);
>    return "prepend\t%0,%z2,%3";
>  }
>    [(set_attr "type"	"arith")

This part's obviously correct though, thanks.  Applied as below.

(Since it isn't a regression, I suppose it shouldn't strictly speaking
be applied during stage 4.  It's very isolated and fixes an obvious typo
though, so I hope no-one minds that I'm making an exception.)

Richard


gcc/
2012-02-02  Jia Liu  <proljc@gmail.com>

	* config/mips/mips-dspr2.md (mips_prepend): Mask operand 3 rather
	than operand 2.

gcc/testsuite/
	* gcc.target/mips/mips-prepend-1.c: New test.

Comments

Fu, Chao-Ying Feb. 3, 2012, 4:40 a.m. UTC | #1 
Richard Sandiford [rdsandiford@googlemail.com] wrote:

> This pattern maps directly to __builtin_mips_prepend, which is defined
> to take and return an SI type, so :SI is the correct choice here.
>
> I agree it might be nice to have a function that operates on 64-bit
> values for 64-bit targets though.  For compatibility reasons,
> we'd probably have to define both a new function and a new pattern
> (or at least an iterator version of the current pattern).
> There's currently no way of generating PREPENDD or PREPENDW either.
>
> I consider this API to be owned by MTI, so we'd need to coordinate
> with them.  Chao-Ying, what do you think?  Do you already have
> something like this internally?

  As there is no MIPS64 core with dsp/dspr2 in the market, we haven't tested/ported these built-in functions to MIPS64.

  I think it's better to keep the current prototype of __builtin_mips_prepend to return an SI type.
We may need to add new built-in functions for instructions involving accumulators and for new MIPS64 dsp/dspr2 instructions to MIPS64.
For existing built-in functions involving only integer registers/immediates, we may not change them.
For Liu, is this solution suitable for you?   Thanks!

Regards,
Chao-ying
Jia Liu Feb. 3, 2012, 6:47 a.m. UTC | #2 
On Fri, Feb 3, 2012 at 12:40 PM, Fu, Chao-Ying <fu@mips.com> wrote:
> Richard Sandiford [rdsandiford@googlemail.com] wrote:
>
>> This pattern maps directly to __builtin_mips_prepend, which is defined
>> to take and return an SI type, so :SI is the correct choice here.
>>
>> I agree it might be nice to have a function that operates on 64-bit
>> values for 64-bit targets though.  For compatibility reasons,
>> we'd probably have to define both a new function and a new pattern
>> (or at least an iterator version of the current pattern).
>> There's currently no way of generating PREPENDD or PREPENDW either.
>>
>> I consider this API to be owned by MTI, so we'd need to coordinate
>> with them.  Chao-Ying, what do you think?  Do you already have
>> something like this internally?
>
>  As there is no MIPS64 core with dsp/dspr2 in the market, we haven't tested/ported these built-in functions to MIPS64.
>
>  I think it's better to keep the current prototype of __builtin_mips_prepend to return an SI type.
> We may need to add new built-in functions for instructions involving accumulators and for new MIPS64 dsp/dspr2 instructions to MIPS64.
> For existing built-in functions involving only integer registers/immediates, we may not change them.
> For Liu, is this solution suitable for you?   Thanks!
>

OK, I get.
But, sorry, my mips64dspr2 patch has be done...
Should I summit it?

> Regards,
> Chao-ying
Fu, Chao-Ying Feb. 3, 2012, 7:59 a.m. UTC | #3 
Liu [proljc@gmail.com] wrote:

> OK, I get.
> But, sorry, my mips64dspr2 patch has be done...
> Should I summit it?

  I just wonder what version of the MIPS64 DSP/DSPr2 spec that you implemented.  Do you have a target CPU that has these MIPS64 DSP/DSPr2 instructions?  My concern is that the latest spec removed a lot of new instructions.  Thanks!

Regards,
Chao-ying
Jia Liu Feb. 3, 2012, 8:43 a.m. UTC | #4 
On Fri, Feb 3, 2012 at 3:59 PM, Fu, Chao-Ying <fu@mips.com> wrote:
> Liu [proljc@gmail.com] wrote:
>
>> OK, I get.
>> But, sorry, my mips64dspr2 patch has be done...
>> Should I summit it?
>
>  I just wonder what version of the MIPS64 DSP/DSPr2 spec that you implemented.  Do you have a target CPU that has these MIPS64 DSP/DSPr2 instructions?  My concern is that the latest spec removed a lot of new instructions.  Thanks!
>

MIPS® Architecture for Programmers
VolumeIV-e: The MIPS® DSP Application-
Specific Extension to the MIPS64®
Architecture
Document Number: MD00375
Revision 2.34
May 6, 2011

I have lots of MIPS64 CPU but no DSP:(

> Regards,
> Chao-ying
diff mbox

Patch

Index: gcc/config/mips/mips-dspr2.md
===================================================================
--- gcc/config/mips/mips-dspr2.md	2012-02-02 18:32:00.000000000 +0000
+++ gcc/config/mips/mips-dspr2.md	2012-02-02 18:32:11.000000000 +0000
@@ -353,7 +353,7 @@  (define_insn "mips_prepend"
   "ISA_HAS_DSPR2"
 {
   if (INTVAL (operands[3]) & ~(unsigned HOST_WIDE_INT) 31)
-    operands[2] = GEN_INT (INTVAL (operands[2]) & 31);
+    operands[3] = GEN_INT (INTVAL (operands[3]) & 31);
   return "prepend\t%0,%z2,%3";
 }
   [(set_attr "type"	"arith")
Index: gcc/testsuite/gcc.target/mips/mips-prepend-1.c
===================================================================
--- /dev/null	2012-02-02 18:24:25.322640797 +0000
+++ gcc/testsuite/gcc.target/mips/mips-prepend-1.c	2012-02-02 18:31:04.000000000 +0000
@@ -0,0 +1,8 @@ 
+/* { dg-options "-mdspr2" } */
+/* { dg-final { scan-assembler "prepend\[^\n\]*,10" } } */
+
+NOMIPS16 int
+foo (int x, int y)
+{
+  return __builtin_mips_prepend (x, y, 42);
+}