[patch] Handle MIPS DSP control registers

[patch] Handle MIPS DSP control registers

[Catherine Moore]

[-- Attachment #1: Type: text/plain, Size: 387 bytes --]

This patch adds definitons for the DSP control registers and then handles them in registers in 
mips_conditional_register_usage.  Okay to install?  Thanks, Catherine


2008-10-22  Catherine Moore  <clm@codesourcery.com>

	* config/mips.h (DSP_CTRL_REG_FIRST): Define.
	(DSP_CTRL_REG_LAST): Define.
	* config/mips.c (mips_conditional_register_usage):  Handle the
	DSP control registers.


[-- Attachment #2: dsp2.patch --]
[-- Type: text/x-patch, Size: 1220 bytes --]

*** mips.c	(revision 225707)
--- mips.c	(local)
*************** mips_swap_registers (unsigned int i)
*** 13939,13950 ****
--- 13939,13959 ----
  void
  mips_conditional_register_usage (void)
  {
+ 
+   /* These DSP control register fields are global.  */
+   if (ISA_HAS_DSP)
+     {
+       global_regs[CCDSP_PO_REGNUM] = 1;
+       global_regs[CCDSP_SC_REGNUM] = 1;
+     }
    if (!ISA_HAS_DSP)
      {
        int regno;
  
        for (regno = DSP_ACC_REG_FIRST; regno <= DSP_ACC_REG_LAST; regno++)
  	fixed_regs[regno] = call_used_regs[regno] = 1;
+       for (regno = DSP_CTRL_REG_FIRST; regno <= DSP_CTRL_REG_LAST; regno++)
+ 	fixed_regs[regno] = call_used_regs[regno] = 1;
      }
    if (!TARGET_HARD_FLOAT)
      {
*** mips.h	(revision 225707)
--- mips.h	(local)
*************** enum mips_code_readable_setting {
*** 1637,1642 ****
--- 1637,1645 ----
  #define DSP_ACC_REG_LAST 181
  #define DSP_ACC_REG_NUM (DSP_ACC_REG_LAST - DSP_ACC_REG_FIRST + 1)
  
+ #define DSP_CTRL_REG_FIRST 182
+ #define DSP_CTRL_REG_LAST 187
+ 
  #define AT_REGNUM	(GP_REG_FIRST + 1)
  #define HI_REGNUM	(TARGET_BIG_ENDIAN ? MD_REG_FIRST : MD_REG_FIRST + 1)
  #define LO_REGNUM	(TARGET_BIG_ENDIAN ? MD_REG_FIRST + 1 : MD_REG_FIRST)

Re: [patch] Handle MIPS DSP control registers

[Richard Sandiford]

Catherine Moore <clm@codesourcery.com> writes:

> This patch adds definitons for the DSP control registers and then handles them in registers in 
> mips_conditional_register_usage.  Okay to install?  Thanks, Catherine
>
>
> 2008-10-22  Catherine Moore  <clm@codesourcery.com>
>
> 	* config/mips.h (DSP_CTRL_REG_FIRST): Define.
> 	(DSP_CTRL_REG_LAST): Define.
> 	* config/mips.c (mips_conditional_register_usage):  Handle the
> 	DSP control registers.
>
> *** mips.c	(revision 225707)
> --- mips.c	(local)
> *************** mips_swap_registers (unsigned int i)
> *** 13939,13950 ****
> --- 13939,13959 ----
>   void
>   mips_conditional_register_usage (void)
>   {
> + 
> +   /* These DSP control register fields are global.  */
> +   if (ISA_HAS_DSP)
> +     {
> +       global_regs[CCDSP_PO_REGNUM] = 1;
> +       global_regs[CCDSP_SC_REGNUM] = 1;
> +     }
>     if (!ISA_HAS_DSP)
>       {
>         int regno;
>   
>         for (regno = DSP_ACC_REG_FIRST; regno <= DSP_ACC_REG_LAST; regno++)
>   	fixed_regs[regno] = call_used_regs[regno] = 1;
> +       for (regno = DSP_CTRL_REG_FIRST; regno <= DSP_CTRL_REG_LAST; regno++)
> + 	fixed_regs[regno] = call_used_regs[regno] = 1;
>       }
>     if (!TARGET_HARD_FLOAT)
>       {
> *** mips.h	(revision 225707)
> --- mips.h	(local)
> *************** enum mips_code_readable_setting {
> *** 1637,1642 ****
> --- 1637,1645 ----
>   #define DSP_ACC_REG_LAST 181
>   #define DSP_ACC_REG_NUM (DSP_ACC_REG_LAST - DSP_ACC_REG_FIRST + 1)
>   
> + #define DSP_CTRL_REG_FIRST 182
> + #define DSP_CTRL_REG_LAST 187
> + 
>   #define AT_REGNUM	(GP_REG_FIRST + 1)
>   #define HI_REGNUM	(TARGET_BIG_ENDIAN ? MD_REG_FIRST : MD_REG_FIRST + 1)
>   #define LO_REGNUM	(TARGET_BIG_ENDIAN ? MD_REG_FIRST + 1 : MD_REG_FIRST)

I'm not saying the patch is wrong, but I'm not sure what the
motivation is.  What goes wrong if we don't fix these registers?
And why do we want to make the SCOUNT and POS fields global?
Why aren't those two fields call-clobbered like the others?

This would be an ABI change from 4.3, so would need to be mentioned
somewhere.  The DSP part of extend.texi should also mention which
fields are global.

Richard

Re: [patch] Handle MIPS DSP control registers

[Chao-ying Fu]

[-- Attachment #1: Type: text/plain, Size: 3504 bytes --]

Richard Sandiford wrote:

> Catherine Moore <clm@codesourcery.com> writes:
>
> > This patch adds definitons for the DSP control registers and then
handles them in registers in
> > mips_conditional_register_usage.  Okay to install?  Thanks, Catherine
> >
> >
> > 2008-10-22  Catherine Moore  <clm@codesourcery.com>
> >
> > * config/mips.h (DSP_CTRL_REG_FIRST): Define.
> > (DSP_CTRL_REG_LAST): Define.
> > * config/mips.c (mips_conditional_register_usage):  Handle the
> > DSP control registers.
> >
> > *** mips.c (revision 225707)
> > --- mips.c (local)
> > *************** mips_swap_registers (unsigned int i)
> > *** 13939,13950 ****
> > --- 13939,13959 ----
> >   void
> >   mips_conditional_register_usage (void)
> >   {
> > +
> > +   /* These DSP control register fields are global.  */
> > +   if (ISA_HAS_DSP)
> > +     {
> > +       global_regs[CCDSP_PO_REGNUM] = 1;
> > +       global_regs[CCDSP_SC_REGNUM] = 1;
> > +     }
> >     if (!ISA_HAS_DSP)
> >       {
> >         int regno;
> >
> >         for (regno = DSP_ACC_REG_FIRST; regno <= DSP_ACC_REG_LAST;
regno++)
> >   fixed_regs[regno] = call_used_regs[regno] = 1;
> > +       for (regno = DSP_CTRL_REG_FIRST; regno <= DSP_CTRL_REG_LAST;
regno++)
> > + fixed_regs[regno] = call_used_regs[regno] = 1;
> >       }
> >     if (!TARGET_HARD_FLOAT)
> >       {
> > *** mips.h (revision 225707)
> > --- mips.h (local)
> > *************** enum mips_code_readable_setting {
> > *** 1637,1642 ****
> > --- 1637,1645 ----
> >   #define DSP_ACC_REG_LAST 181
> >   #define DSP_ACC_REG_NUM (DSP_ACC_REG_LAST - DSP_ACC_REG_FIRST + 1)
> >
> > + #define DSP_CTRL_REG_FIRST 182
> > + #define DSP_CTRL_REG_LAST 187
> > +
> >   #define AT_REGNUM (GP_REG_FIRST + 1)
> >   #define HI_REGNUM (TARGET_BIG_ENDIAN ? MD_REG_FIRST : MD_REG_FIRST +
1)
> >   #define LO_REGNUM (TARGET_BIG_ENDIAN ? MD_REG_FIRST + 1 :
MD_REG_FIRST)
>
> I'm not saying the patch is wrong, but I'm not sure what the
> motivation is.  What goes wrong if we don't fix these registers?
> And why do we want to make the SCOUNT and POS fields global?
> Why aren't those two fields call-clobbered like the others?
>
> This would be an ABI change from 4.3, so would need to be mentioned
> somewhere.  The DSP part of extend.texi should also mention which
> fields are global.
>
> Richard
>

  Back in Nov 16, 2005, Nigel Stephens, David Ung, Radhika Thekkath, and I
discussed
the usage of DSP control registers and had the conclusion as follows.
-----
1. Function calls are assumed to clobber all fields of

the DSP control register. The compiler must make sure that

DSP instructions that read any field of the DSP

control register are not moved before or after function calls.

2. SCOUNT and POS of the DSP control register are global.

The compiler cannot delete instructions that modify

SCOUNT or POS in any situation. These instructions

include WRDSP, EXTPDP, EXTPDPV, and MTHLIP.

Also, the compiler cannot delete any function calls

that jump to a function containing WRDSP, EXTPDP,

EXTPDPV, or MTHLIP.

3. Based on 1 and 2, programmers must know that after a function

call, intrinsics that depend on SCOUNT and POS can be used.

But, there is no guarantee that four other fields (CCOND, OUFLAG, EFI, C)

of the DSP control register are valid, so programmers

should re-initialize the values of (CCOND, OUFLAG, EFI, C)

if these fields are needed by following intrinsics.

-----

  One test was created to cover the behavior of DSP control registers.

Thanks!

Regards,

Chao-ying

[-- Attachment #2: mips-dspcntl.c --]
[-- Type: text/plain, Size: 1275 bytes --]

/* { dg-do run { target mips*-*-* } } */
/* { dg-options "" } */

#if __mips_dsp

void test1 (int i)
{
  __builtin_mips_wrdsp (i, 63);
}

void test2 ()
{
  long long a = 0;
  __builtin_mips_extpdp (a, 3);
}

void test3 (int i)
{
  long long a = 0;
  __builtin_mips_extpdp (a, i);
}

void test4 ()
{
  long long a = 0;
  int i = 0;
  __builtin_mips_mthlip (a, i);
}

int main()
{
  int cntl;

  /* Test 1: wrdsp */
  __builtin_mips_wrdsp (0,63);
  test1 (63);
  cntl = __builtin_mips_rddsp (63);
  if (cntl != 63)
  {
    printf ("cntl=%d\n", cntl);
    abort ();
  }

  /* Test 2: extpdp */
  __builtin_mips_wrdsp (63,63);
  test2 ();
  cntl = __builtin_mips_rddsp (63);
  if (cntl != 59)
  {
    printf ("cntl=%d\n", cntl);
    abort ();
  }

  /* Test 3: extpdpv */
  __builtin_mips_wrdsp (63,63);
  test3 (10);
  cntl = __builtin_mips_rddsp (63);
  if (cntl != 52)
  {
    printf ("cntl=%d\n", cntl);
    abort ();
  }

  /* Test 4: mthlip */
  __builtin_mips_wrdsp (8,63);
  test4 ();
  cntl = __builtin_mips_rddsp (63);
  if (cntl != 40)
  {
    printf ("cntl=%d\n", cntl);
    abort ();
  }

  exit (0);
}

#else

int main ()
{
  printf ("Unsupported test\n");
  exit (0); 
}

#endif

Re: [patch] Handle MIPS DSP control registers

[Richard Sandiford]

Hi Chao-ying,

"Chao-ying Fu" <fu@mips.com> writes:
> Richard Sandiford wrote:
>> Catherine Moore <clm@codesourcery.com> writes:
>>
>> > This patch adds definitons for the DSP control registers and then
> handles them in registers in
>> > mips_conditional_register_usage.  Okay to install?  Thanks, Catherine
>> >
>> >
>> > 2008-10-22  Catherine Moore  <clm@codesourcery.com>
>> >
>> > * config/mips.h (DSP_CTRL_REG_FIRST): Define.
>> > (DSP_CTRL_REG_LAST): Define.
>> > * config/mips.c (mips_conditional_register_usage):  Handle the
>> > DSP control registers.
>> >
>> > *** mips.c (revision 225707)
>> > --- mips.c (local)
>> > *************** mips_swap_registers (unsigned int i)
>> > *** 13939,13950 ****
>> > --- 13939,13959 ----
>> >   void
>> >   mips_conditional_register_usage (void)
>> >   {
>> > +
>> > +   /* These DSP control register fields are global.  */
>> > +   if (ISA_HAS_DSP)
>> > +     {
>> > +       global_regs[CCDSP_PO_REGNUM] = 1;
>> > +       global_regs[CCDSP_SC_REGNUM] = 1;
>> > +     }
>> >     if (!ISA_HAS_DSP)
>> >       {
>> >         int regno;
>> >
>> >         for (regno = DSP_ACC_REG_FIRST; regno <= DSP_ACC_REG_LAST;
> regno++)
>> >   fixed_regs[regno] = call_used_regs[regno] = 1;
>> > +       for (regno = DSP_CTRL_REG_FIRST; regno <= DSP_CTRL_REG_LAST;
> regno++)
>> > + fixed_regs[regno] = call_used_regs[regno] = 1;
>> >       }
>> >     if (!TARGET_HARD_FLOAT)
>> >       {
>> > *** mips.h (revision 225707)
>> > --- mips.h (local)
>> > *************** enum mips_code_readable_setting {
>> > *** 1637,1642 ****
>> > --- 1637,1645 ----
>> >   #define DSP_ACC_REG_LAST 181
>> >   #define DSP_ACC_REG_NUM (DSP_ACC_REG_LAST - DSP_ACC_REG_FIRST + 1)
>> >
>> > + #define DSP_CTRL_REG_FIRST 182
>> > + #define DSP_CTRL_REG_LAST 187
>> > +
>> >   #define AT_REGNUM (GP_REG_FIRST + 1)
>> >   #define HI_REGNUM (TARGET_BIG_ENDIAN ? MD_REG_FIRST : MD_REG_FIRST +
> 1)
>> >   #define LO_REGNUM (TARGET_BIG_ENDIAN ? MD_REG_FIRST + 1 :
> MD_REG_FIRST)
>>
>> I'm not saying the patch is wrong, but I'm not sure what the
>> motivation is.  What goes wrong if we don't fix these registers?
>> And why do we want to make the SCOUNT and POS fields global?
>> Why aren't those two fields call-clobbered like the others?
>>
>> This would be an ABI change from 4.3, so would need to be mentioned
>> somewhere.  The DSP part of extend.texi should also mention which
>> fields are global.
>>
>> Richard
>>
>
>   Back in Nov 16, 2005, Nigel Stephens, David Ung, Radhika Thekkath, and I
> discussed
> the usage of DSP control registers and had the conclusion as follows.
> -----
> 1. Function calls are assumed to clobber all fields of
> the DSP control register. The compiler must make sure that
> DSP instructions that read any field of the DSP
> control register are not moved before or after function calls.
>
> 2. SCOUNT and POS of the DSP control register are global.
> The compiler cannot delete instructions that modify
> SCOUNT or POS in any situation. These instructions
> include WRDSP, EXTPDP, EXTPDPV, and MTHLIP.
> Also, the compiler cannot delete any function calls
> that jump to a function containing WRDSP, EXTPDP,
> EXTPDPV, or MTHLIP.
>
> 3. Based on 1 and 2, programmers must know that after a function
> call, intrinsics that depend on SCOUNT and POS can be used.
> But, there is no guarantee that four other fields (CCOND, OUFLAG, EFI, C)
> of the DSP control register are valid, so programmers
> should re-initialize the values of (CCOND, OUFLAG, EFI, C)
> if these fields are needed by following intrinsics.
> -----
>
>   One test was created to cover the behavior of DSP control registers.

OK, thanks for the info.

(It's a little unfortunate that the ABI was changed four months after
the DSP support was added to FSF GCC, yet it seems to have taken three
years for this decision to reach an FSF forum.  But there's nothing
to be done about that now.)

As far as the patch itself goes: the DSP registers are already
(unconditionally) fixed and call-clobbered, so the second part of the
patch should be redundant.  (The registers should be fixed because
they aren't allocatable even when DSP support is enabled.  And as
Chao-ying's message says, they're defined to be call-clobbered.)

As I said above, I think we should document the globalness of SCOUNT
and POS in the DSP part of extend.texi.  And because we're changing
the ABI, I think this deserves a mention in the "Caveats" section of
gcc-4.4/changes.html.

We should take the test Chao-Ying posted and add it to gcc.target/mips.
(I believe it's covered by MTI's copyright assignment.)  I think it
would be better to run the test with -O2 and mark the subroutines as
"noinline"; that should stress more RTL code without compromising
the test.

Very minor nit, sorry, but I'd prefer if (ISA_HAS_DSP) ...; else ...;
over if (ISA_HAS_DSP) ...; if (!ISA_HAS_DSP) ...;

Richard

Re: [patch] Handle MIPS DSP control registers

[Catherine Moore]

Hi Richard,

Here is the updated patch which incorporates your comments.  I will send the wwwdocs patch in a 
separate message.  Does this look okay to install?   Thanks,  Catherine

2008-10-30  Catherine Moore  <clm@codesourcery.com>

  	* config/mips.h (DSP_CTRL_REG_FIRST): Define.
  	(DSP_CTRL_REG_LAST): Define.
  	* config/mips.c (mips_conditional_register_usage):  Handle the
  	DSP control registers.
	* doc/extend.texi
	
2008-10-30  Chao-ying Fu  <fu@mips.com>

	* gcc.target/mips/dsp-ctrl.c: New test.

Index: config/mips/mips.c
===================================================================
--- config/mips/mips.c	(revision 141304)
+++ config/mips/mips.c	(working copy)
@@ -13939,7 +13939,14 @@ mips_swap_registers (unsigned int i)
  void
  mips_conditional_register_usage (void)
  {
-  if (!ISA_HAS_DSP)
+
+  /* These DSP control register fields are global.  */
+  if (ISA_HAS_DSP)
+    {
+      global_regs[CCDSP_PO_REGNUM] = 1;
+      global_regs[CCDSP_SC_REGNUM] = 1;
+    }
+  else
      {
        int regno;

Index: config/mips/mips.h
===================================================================
--- config/mips/mips.h	(revision 141150)
+++ config/mips/mips.h	(working copy)
@@ -1637,6 +1637,9 @@ enum mips_code_readable_setting {
  #define DSP_ACC_REG_LAST 181
  #define DSP_ACC_REG_NUM (DSP_ACC_REG_LAST - DSP_ACC_REG_FIRST + 1)

+#define DSP_CTRL_REG_FIRST 182
+#define DSP_CTRL_REG_LAST 187
+
  #define AT_REGNUM	(GP_REG_FIRST + 1)
  #define HI_REGNUM	(TARGET_BIG_ENDIAN ? MD_REG_FIRST : MD_REG_FIRST + 1)
  #define LO_REGNUM	(TARGET_BIG_ENDIAN ? MD_REG_FIRST + 1 : MD_REG_FIRST)
Index: doc/extend.texi
===================================================================
--- doc/extend.texi	(revision 141150)
+++ doc/extend.texi	(working copy)
@@ -8698,6 +8698,12 @@ otherwise backwards-compatible with it.
  using the command-line option @option{-mdspr2}; this option implies
  @option{-mdsp}.

+The SCOUNT and POS bits of the DSP control register are global.  The
+WRDSP, EXTPDP, EXTPDPV and MTHLIP instructions modify the SCOUNT and
+POS bits.  During optimization, the compiler will not delete these
+instructions and it will not delete calls to functions containing
+these instructions.
+
  At present, GCC only provides support for operations on 32-bit
  vectors.  The vector type associated with 8-bit integer data is
  usually called @code{v4i8}, the vector type associated with Q7
Index: testsuite/gcc.target/mips/dsp-ctrl.c
===================================================================
--- testsuite/gcc.target/mips/dsp-ctrl.c	(revision 0)
+++ testsuite/gcc.target/mips/dsp-ctrl.c	(revision 0)
@@ -0,0 +1,80 @@
+/* { dg-do run { target mips*-*-* } } */
+/* { dg-options "-O2" } */
+
+extern void abort (void);
+extern void exit (int);
+#if __mips_dsp
+
+void __attribute__ ((noinline))
+test1 (int i)
+{
+  __builtin_mips_wrdsp (i, 63);
+}
+
+void __attribute__ ((noinline))
+test2 ()
+{
+  long long a = 0;
+  __builtin_mips_extpdp (a, 3);
+}
+
+void __attribute__ ((noinline))
+test3 (int i)
+{
+  long long a = 0;
+  __builtin_mips_extpdp (a, i);
+}
+
+void __attribute__ ((noinline))
+test4 ()
+{
+  long long a = 0;
+  int i = 0;
+  __builtin_mips_mthlip (a, i);
+}
+
+int
+main ()
+{
+  int cntl;
+
+  /* Test 1: wrdsp */
+  __builtin_mips_wrdsp (0,63);
+  test1 (63);
+  cntl = __builtin_mips_rddsp (63);
+  if (cntl != 63)
+    abort ();
+
+  /* Test 2: extpdp */
+  __builtin_mips_wrdsp (63,63);
+  test2 ();
+  cntl = __builtin_mips_rddsp (63);
+  if (cntl != 59)
+    abort ();
+
+  /* Test 3: extpdpv */
+  __builtin_mips_wrdsp (63,63);
+  test3 (10);
+  cntl = __builtin_mips_rddsp (63);
+  if (cntl != 52)
+    abort ();
+
+  /* Test 4: mthlip */
+  __builtin_mips_wrdsp (8,63);
+  test4 ();
+  cntl = __builtin_mips_rddsp (63);
+  if (cntl != 40)
+    abort ();
+
+  exit (0);
+}
+
+#else
+
+int
+main ()
+{
+  exit (0);
+}
+
+#endif


Richard Sandiford wrote:
> Hi Chao-ying,
> 
> "Chao-ying Fu" <fu@mips.com> writes:
>> Richard Sandiford wrote:
>>> Catherine Moore <clm@codesourcery.com> writes:
>>>
>>>> This patch adds definitons for the DSP control registers and then
>> handles them in registers in
>>>> mips_conditional_register_usage.  Okay to install?  Thanks, Catherine
>>>>
>>>>
>>>> 2008-10-22  Catherine Moore  <clm@codesourcery.com>
>>>>
>>>> * config/mips.h (DSP_CTRL_REG_FIRST): Define.
>>>> (DSP_CTRL_REG_LAST): Define.
>>>> * config/mips.c (mips_conditional_register_usage):  Handle the
>>>> DSP control registers.
>>>>
>>>> *** mips.c (revision 225707)
>>>> --- mips.c (local)
>>>> *************** mips_swap_registers (unsigned int i)
>>>> *** 13939,13950 ****
>>>> --- 13939,13959 ----
>>>>   void
>>>>   mips_conditional_register_usage (void)
>>>>   {
>>>> +
>>>> +   /* These DSP control register fields are global.  */
>>>> +   if (ISA_HAS_DSP)
>>>> +     {
>>>> +       global_regs[CCDSP_PO_REGNUM] = 1;
>>>> +       global_regs[CCDSP_SC_REGNUM] = 1;
>>>> +     }
>>>>     if (!ISA_HAS_DSP)
>>>>       {
>>>>         int regno;
>>>>
>>>>         for (regno = DSP_ACC_REG_FIRST; regno <= DSP_ACC_REG_LAST;
>> regno++)
>>>>   fixed_regs[regno] = call_used_regs[regno] = 1;
>>>> +       for (regno = DSP_CTRL_REG_FIRST; regno <= DSP_CTRL_REG_LAST;
>> regno++)
>>>> + fixed_regs[regno] = call_used_regs[regno] = 1;
>>>>       }
>>>>     if (!TARGET_HARD_FLOAT)
>>>>       {
>>>> *** mips.h (revision 225707)
>>>> --- mips.h (local)
>>>> *************** enum mips_code_readable_setting {
>>>> *** 1637,1642 ****
>>>> --- 1637,1645 ----
>>>>   #define DSP_ACC_REG_LAST 181
>>>>   #define DSP_ACC_REG_NUM (DSP_ACC_REG_LAST - DSP_ACC_REG_FIRST + 1)
>>>>
>>>> + #define DSP_CTRL_REG_FIRST 182
>>>> + #define DSP_CTRL_REG_LAST 187
>>>> +
>>>>   #define AT_REGNUM (GP_REG_FIRST + 1)
>>>>   #define HI_REGNUM (TARGET_BIG_ENDIAN ? MD_REG_FIRST : MD_REG_FIRST +
>> 1)
>>>>   #define LO_REGNUM (TARGET_BIG_ENDIAN ? MD_REG_FIRST + 1 :
>> MD_REG_FIRST)
>>> I'm not saying the patch is wrong, but I'm not sure what the
>>> motivation is.  What goes wrong if we don't fix these registers?
>>> And why do we want to make the SCOUNT and POS fields global?
>>> Why aren't those two fields call-clobbered like the others?
>>>
>>> This would be an ABI change from 4.3, so would need to be mentioned
>>> somewhere.  The DSP part of extend.texi should also mention which
>>> fields are global.
>>>
>>> Richard
>>>
>>   Back in Nov 16, 2005, Nigel Stephens, David Ung, Radhika Thekkath, and I
>> discussed
>> the usage of DSP control registers and had the conclusion as follows.
>> -----
>> 1. Function calls are assumed to clobber all fields of
>> the DSP control register. The compiler must make sure that
>> DSP instructions that read any field of the DSP
>> control register are not moved before or after function calls.
>>
>> 2. SCOUNT and POS of the DSP control register are global.
>> The compiler cannot delete instructions that modify
>> SCOUNT or POS in any situation. These instructions
>> include WRDSP, EXTPDP, EXTPDPV, and MTHLIP.
>> Also, the compiler cannot delete any function calls
>> that jump to a function containing WRDSP, EXTPDP,
>> EXTPDPV, or MTHLIP.
>>
>> 3. Based on 1 and 2, programmers must know that after a function
>> call, intrinsics that depend on SCOUNT and POS can be used.
>> But, there is no guarantee that four other fields (CCOND, OUFLAG, EFI, C)
>> of the DSP control register are valid, so programmers
>> should re-initialize the values of (CCOND, OUFLAG, EFI, C)
>> if these fields are needed by following intrinsics.
>> -----
>>
>>   One test was created to cover the behavior of DSP control registers.
> 
> OK, thanks for the info.
> 
> (It's a little unfortunate that the ABI was changed four months after
> the DSP support was added to FSF GCC, yet it seems to have taken three
> years for this decision to reach an FSF forum.  But there's nothing
> to be done about that now.)
> 
> As far as the patch itself goes: the DSP registers are already
> (unconditionally) fixed and call-clobbered, so the second part of the
> patch should be redundant.  (The registers should be fixed because
> they aren't allocatable even when DSP support is enabled.  And as
> Chao-ying's message says, they're defined to be call-clobbered.)
> 
> As I said above, I think we should document the globalness of SCOUNT
> and POS in the DSP part of extend.texi.  And because we're changing
> the ABI, I think this deserves a mention in the "Caveats" section of
> gcc-4.4/changes.html.
> 
> We should take the test Chao-Ying posted and add it to gcc.target/mips.
> (I believe it's covered by MTI's copyright assignment.)  I think it
> would be better to run the test with -O2 and mark the subroutines as
> "noinline"; that should stress more RTL code without compromising
> the test.
> 
> Very minor nit, sorry, but I'd prefer if (ISA_HAS_DSP) ...; else ...;
> over if (ISA_HAS_DSP) ...; if (!ISA_HAS_DSP) ...;
> 
> Richard

Re: [patch] Handle MIPS DSP control registers

[Richard Sandiford]

Catherine Moore <clm@codesourcery.com> writes:
> Here is the updated patch which incorporates your comments.  I will
> send the wwwdocs patch in a separate message.  Does this look okay to
> install?

Looks good, thanks.  Some comments below...

> -  if (!ISA_HAS_DSP)
> +
> +  /* These DSP control register fields are global.  */
> +  if (ISA_HAS_DSP)
> +    {
> +      global_regs[CCDSP_PO_REGNUM] = 1;
> +      global_regs[CCDSP_SC_REGNUM] = 1;
> +    }
> +  else

Very minor nit, but the comment ought to go directly above the
global_regs assignments, since it doesn't apply when !ISA_HAS_DSP.
I'd prefer:

  /* The SCOUNT and POS fields are global.  */

> +#define DSP_CTRL_REG_FIRST 182
> +#define DSP_CTRL_REG_LAST 187

The new patch doesn't need these macros.

> +The SCOUNT and POS bits of the DSP control register are global.  The
> +WRDSP, EXTPDP, EXTPDPV and MTHLIP instructions modify the SCOUNT and
> +POS bits.  During optimization, the compiler will not delete these
> +instructions and it will not delete calls to functions containing
> +these instructions.

I think we generally mark instruction names like WRDSP as:

  @code{wrdsp}

(SCOUNT and POS are OK as they are.)

OK with those changes, thanks.

Richard