[RFA] sh-sim: restructure expand_opcode.

[RFA] sh-sim: restructure expand_opcode.

[Michael Snyder]

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

Hi Joern,

This supercedes my last patch.  All I'm doing in this one is
expand_opcode, implementing some of the changes we discussed
(to pave the way for the sh4a patch to come).  It's much
simpler, and fewer lines of code.

It now accepts either 2 or 4 bits at a time (except for ones
and zeroes, which accept an arbitrary sequence).  The sub-
cases such as 'N' and 'x' are moved out to the outer switch
(so its flatter and simpler).  The "shift" parameter is gone
(which required that the movx/movy expansion be moved into
the post-processing pass in ppi_moves).  "ddd0" becomes
"eeee" (even numbered register), and I don't check it at runtime.

The XY/xy stuff will come in the next patch.

Michael


[-- Attachment #2: joern --]
[-- Type: text/plain, Size: 9452 bytes --]

2003-12-18  Michael Snyder  <msnyder@redhat.com>

	* gencode.c (expand_opcode): Simplify and reorganize.
	Eliminate "shift" parameter.  Eliminate "4 bits at a time"
	assumption.  Flatten switch statement to a single level.
	Add "eeee" token for even-numbered registers.
	(bton): Delete.
	(fsca): Use "eeee" token.
	(ppi_moves): Rename to "expand_ppi_movxy".  Do the ddt
	[movx/movy] expansion here, as well as the ppi expansion.
	(gensim_caselist): Accept 'eeee' along with 'nnnn'.

Index: gencode.c
===================================================================
RCS file: /cvs/src/src/sim/sh/gencode.c,v
retrieving revision 1.21
diff -p -r1.21 gencode.c
*** gencode.c	3 Nov 2003 14:14:15 -0000	1.21
--- gencode.c	19 Dec 2003 03:43:05 -0000
*************** op tab[] =
*** 449,455 ****
    },
  
    /* sh4 */
!   { "", "", "fsca", "1111nnn011111101",
      "if (FPSCR_PR)",
      "  RAISE_EXCEPTION (SIGILL);",
      "else",
--- 449,455 ----
    },
  
    /* sh4 */
!   { "", "", "fsca", "1111eeee11111101",
      "if (FPSCR_PR)",
      "  RAISE_EXCEPTION (SIGILL);",
      "else",
*************** gengastab ()
*** 1950,2077 ****
  
  }
  
- /* Convert a string of 4 binary digits into an int */
- 
- static
- int
- bton (s)
-      char *s;
- 
- {
-   int n = 0;
-   int v = 8;
-   while (v)
-     {
-       if (*s == '1')
- 	n |= v;
-       v >>= 1;
-       s++;
-     }
-   return n;
- }
- 
  static unsigned char table[1 << 16];
  
! /* Take an opcode expand all varying fields in it out and fill all the
!   right entries in 'table' with the opcode index*/
  
  static void
! expand_opcode (shift, val, i, s)
!      int shift;
       int val;
       int i;
       char *s;
  {
-   int j;
- 
    if (*s == 0)
      {
        table[val] = i;
      }
    else
      {
        switch (s[0])
  	{
! 
  	case '0':
  	case '1':
! 	  {
! 	    int m, mv;
! 
! 	    if (s[1] - '0' > 1U || !s[2] || ! s[3])
! 	      expand_opcode (shift - 1, val + s[0] - '0', i, s + 1);
! 	    val |= bton (s) << shift;
! 	    if (s[2] == '0' || s[2] == '1')
! 	      expand_opcode (shift - 4, val, i, s + 4);
! 	    else if (s[2] == 'N')
! 	      for (j = 0; j < 4; j++)
! 		expand_opcode (shift - 4, val | (j << shift), i, s + 4);
! 	    else if (s[2] == 'x')
! 	      for (j = 0; j < 4; j += 2)
! 		for (m = 0; m < 32; m++)
! 		  {
! 		    /* Ignore illegal nopy */
! 		    if ((m & 7) == 0 && m != 0)
! 		      continue;
! 		    mv = m & 3 | (m & 4) << 2 | (m & 8) << 3 | (m & 16) << 4;
! 		    expand_opcode (shift - 4, val | mv | (j << shift), i,
! 				   s + 4);
! 		  }
! 	    else if (s[2] == 'y')
! 	      for (j = 0; j < 2; j++)
! 		expand_opcode (shift - 4, val | (j << shift), i, s + 4);
  	    break;
! 	  }
  	case 'n':
  	case 'm':
! 	  {
! 	    int digits = 1;
! 	    while (s[digits] == s[0])
! 	      digits++;
! 	    for (j = 0; j < (1 << digits); j++)
! 	      {
! 		expand_opcode (shift - digits, val | (j << shift), i,
! 			       s + digits);
! 	      }
! 	    break;
! 	  }
  	case 'M':
! 	  /* A1, A0,X0,X1,Y0,Y1,M0,A1G,M1,M1G */
! 	  for (j = 5; j < 16; j++)
! 	    if (j != 6)
! 	      expand_opcode (shift - 4, val | (j << shift), i, s + 4);
  	  break;
  	case 'G':
! 	  /* A1G, A0G: */
  	  for (j = 13; j <= 15; j +=2)
! 	    expand_opcode (shift - 4, val | (j << shift), i, s + 4);
  	  break;
  	case 's':
  	  /* System registers mach, macl, pr: */
  	  for (j = 0; j < 3; j++)
! 	    expand_opcode (shift - 4, val | (j << shift), i, s + 4);
  	  /* System registers fpul, fpscr/dsr, a0, x0, x1, y0, y1: */
  	  for (j = 5; j < 12; j++)
! 	    expand_opcode (shift - 4, val | (j << shift), i, s + 4);
  	  break;
  	case 'X':
  	case 'a':
! 	  val |= bton (s) << shift;
! 	  for (j = 0; j < 16; j += 8)
! 	    expand_opcode (shift - 4, val | (j << shift), i, s + 4);
  	  break;
  	case 'Y':
  	case 'A':
! 	  val |= bton (s) << shift;
! 	  for (j = 0; j < 8; j += 4)
! 	    expand_opcode (shift - 4, val | (j << shift), i, s + 4);
  	  break;
- 
- 	default:
- 	  for (j = 0; j < (1 << (shift + 4)); j++)
- 	    {
- 	      table[val | j] = i;
- 	    }
  	}
      }
  }
--- 1950,2049 ----
  
  }
  
  static unsigned char table[1 << 16];
  
! /* Take an opcode, expand all varying fields in it out and fill all the
!    right entries in 'table' with the opcode index.  */
  
  static void
! expand_opcode (val, i, s)
       int val;
       int i;
       char *s;
  {
    if (*s == 0)
      {
        table[val] = i;
      }
    else
      {
+       int j = 0, m = 0;
+ 
        switch (s[0])
  	{
! 	default:
! 	  fprintf (stderr, "expand_opcode: illegal char '%c'\n", s[0]);
! 	  exit (1);
  	case '0':
  	case '1':
! 	    /* Consume an arbitrary number of ones and zeros.  */
! 	    do {
! 	      j = (j << 1) + (s[m++] - '0');
! 	    } while (s[m] == '0' || s[m] == '1');
! 	    expand_opcode ((val << m) | j, i, s + m);
  	    break;
! 
! 	case 'N':	/* NN -- four-way fork */
! 	  for (j = 0; j < 4; j++)
! 	    expand_opcode ((val << 2) | j, i, s + 2);
! 	  break;
! 	case 'x':	/* xx -- 2-way fork */
! 	  /* Cross-breeding with movy moved to separate function.  */
! 	  for (j = 0; j < 4; j += 2)
! 	    expand_opcode ((val << 2) | j, i, s + 2);
! 	  break;
! 	case 'y':	/* yy -- two-way fork */
! 	  for (j = 0; j < 2; j++)
! 	    expand_opcode ((val << 2) | j, i, s + 2);
! 	  break;
! 	case 'i':	/* eg. "i8*1" */
! 	case '.':	/* "...." is a wildcard */
  	case 'n':
  	case 'm':
! 	  /* nnnn, mmmm, i#*#, .... -- 16-way fork.  */
! 	  for (j = 0; j < 16; j++)
! 	    expand_opcode ((val << 4) | j, i, s + 4);
! 	  break;
! 	case 'e':
! 	  /* eeee -- even-numbered register: 
  	     8 way fork.  */
! 	  for (j = 0; j < 15; j += 2)
! 	    expand_opcode ((val << 4) | j, i, s + 4);
! 	  break;
  	case 'M':
! 	  /* A0, A1, X0, X1, Y0, Y1, M0, M1, A0G, A1G:
! 	     MMMM -- 10-way fork */
! 	  expand_opcode (val | 5, i, s + 4);
! 	  for (j = 7; j < 16; j++)
! 	    expand_opcode ((val << 4) | j, i, s + 4);
  	  break;
  	case 'G':
! 	  /* A1G, A0G: 
! 	     GGGG -- two-way fork */
  	  for (j = 13; j <= 15; j +=2)
! 	    expand_opcode ((val << 4) | j, i, s + 4);
  	  break;
  	case 's':
+ 	  /* ssss -- 10-way fork */
  	  /* System registers mach, macl, pr: */
  	  for (j = 0; j < 3; j++)
! 	    expand_opcode ((val << 4) | j, i, s + 4);
  	  /* System registers fpul, fpscr/dsr, a0, x0, x1, y0, y1: */
  	  for (j = 5; j < 12; j++)
! 	    expand_opcode ((val << 4) | j, i, s + 4);
  	  break;
  	case 'X':
  	case 'a':
! 	  /* XX, aa -- two-way fork */
! 	  for (j = 0; j < 4; j += 2)
! 	    expand_opcode ((val << 2) | j, i, s + 2);
  	  break;
  	case 'Y':
  	case 'A':
! 	  /* YY, AA -- two-way fork */
! 	  for (j = 0; j < 2; j++)
! 	    expand_opcode ((val << 2) | j, i, s + 2);
  	  break;
  	}
      }
  }
*************** filltable (p)
*** 2127,2133 ****
    for (; p->name; p++)
      {
        p->index = index++;
!       expand_opcode (12, 0, p->index, p->code);
      }
  }
  
--- 2099,2105 ----
    for (; p->name; p++)
      {
        p->index = index++;
!       expand_opcode (0, p->index, p->code);
      }
  }
  
*************** filltable (p)
*** 2136,2146 ****
     processing insns (ppi) for code 0xf800 (ppi nopx nopy).  Copy the
     latter tag to represent all combinations of ppi with ddt.  */
  static void
! ppi_moves ()
  {
    int i;
  
    for (i = 0xf000; i < 0xf400; i++)
      if (table[i])
        table[i + 0x800] = table[0xf800];
  }
--- 2108,2136 ----
     processing insns (ppi) for code 0xf800 (ppi nopx nopy).  Copy the
     latter tag to represent all combinations of ppi with ddt.  */
  static void
! expand_ppi_movxy ()
  {
    int i;
  
    for (i = 0xf000; i < 0xf400; i++)
+     if ((i & 3) == 0 && (i & 12) != 0 && table[i] != 0)
+       {
+ 	/* A movx insn, which needs to be filled out with the 
+ 	   corresponding movy insns.  This used to be done in
+ 	   expand_opcode.  */
+ 	int m, mv;
+ 
+ 	for (m = 0; m < 32; m++)
+ 	  {
+ 	    /* Ignore illegal nopy */
+ 	    if ((m & 7) == 0 && m != 0)
+ 	      continue;
+ 	    mv = m & 3 | (m & 4) << 2 | (m & 8) << 3 | (m & 16) << 4;
+ 	    table [i | mv] = table [i];
+ 	  }
+       }
+ 
+   for (i = 0xf000; i < 0xf400; i++)
      if (table[i])
        table[i + 0x800] = table[0xf800];
  }
*************** gensim_caselist (p)
*** 2179,2185 ****
  	      s += 4;
  	      break;
  	    case 'n':
! 	      printf ("      int n = (iword >>8) & 0xf;\n");
  	      needn = 1;
  	      s += 4;
  	      break;
--- 2169,2176 ----
  	      s += 4;
  	      break;
  	    case 'n':
! 	    case 'e':
! 	      printf ("      int n = (iword >> 8) & 0xf;\n");
  	      needn = 1;
  	      s += 4;
  	      break;
*************** gensim_caselist (p)
*** 2202,2208 ****
  	    case 's':
  	    case 'M':
  	    case 'G':
! 	      printf ("      int m = (iword >>4) & 0xf;\n");
  	      s += 4;
  	      break;
  	    case 'X':
--- 2193,2199 ----
  	    case 's':
  	    case 'M':
  	    case 'G':
! 	      printf ("      int m = (iword >> 4) & 0xf;\n");
  	      s += 4;
  	      break;
  	    case 'X':
*************** main (ac, av)
*** 2618,2624 ****
  
  	  memset (table, 0, sizeof table);
  	  filltable (movsxy_tab);
! 	  ppi_moves ();
  	  dumptable ("sh_dsp_table", 1 << 12, 0xf000);
  
  	  memset (table, 0, sizeof table);
--- 2609,2615 ----
  
  	  memset (table, 0, sizeof table);
  	  filltable (movsxy_tab);
! 	  expand_ppi_movxy ();
  	  dumptable ("sh_dsp_table", 1 << 12, 0xf000);
  
  	  memset (table, 0, sizeof table);

Re: [RFA] sh-sim: restructure expand_opcode.

[Joern Rennecke]

Michael Snyder wrote:
> 
> This is a multi-part message in MIME format.
> --------------070904020002040701070808
> Content-Type: text/plain; charset=us-ascii; format=flowed
> Content-Transfer-Encoding: 7bit
> 
> Hi Joern,
> 
> This supercedes my last patch.  All I'm doing in this one is
> expand_opcode, implementing some of the changes we discussed
> (to pave the way for the sh4a patch to come).  It's much
> simpler, and fewer lines of code.
> 
> It now accepts either 2 or 4 bits at a time (except for ones
> and zeroes, which accept an arbitrary sequence).  The sub-
> cases such as 'N' and 'x' are moved out to the outer switch
> (so its flatter and simpler).  The "shift" parameter is gone
> (which required that the movx/movy expansion be moved into
> the post-processing pass in ppi_moves).  "ddd0" becomes
> "eeee" (even numbered register), and I don't check it at runtime.
> 
> The XY/xy stuff will come in the next patch.
> 
> Michael
> 
> 
> --------------070904020002040701070808
> Content-Type: text/plain;
>  name="joern"
> Content-Transfer-Encoding: 7bit
> Content-Disposition: inline;
>  filename="joern"
> 
> 2003-12-18  Michael Snyder  <msnyder@redhat.com>
> 
> 	* gencode.c (expand_opcode): Simplify and reorganize.
> 	Eliminate "shift" parameter.  Eliminate "4 bits at a time"
> 	assumption.  Flatten switch statement to a single level.
> 	Add "eeee" token for even-numbered registers.
> 	(bton): Delete.
> 	(fsca): Use "eeee" token.
> 	(ppi_moves): Rename to "expand_ppi_movxy".  Do the ddt
> 	[movx/movy] expansion here, as well as the ppi expansion.
> 	(gensim_caselist): Accept 'eeee' along with 'nnnn'.

Yes, I agree that this patch improves the code.  Just a few more suggestions /
corrections:

> ! /* Take an opcode, expand all varying fields in it out and fill all the
> !    right entries in 'table' with the opcode index.  */
>   
>   static void
> ! expand_opcode (val, i, s)

This never adhered to the GNU coding standards on comments (unless it preceded
the clause on documenting arguments :-) .  This seems a good opportunity to
fix this.  Something like:

/* Take an opcode, expand all varying fields in it out and fill all the
   right entries in 'table' with the opcode index I.
   VAL is the opcode computed so far, which needs left-shifting by the
   number of bits that are still to be processed.  S is a string describing
   the remaining bits.  */


> +       int j = 0, m = 0;
> + 
>         switch (s[0])
>   	{
> ! 	default:
> ! 	  fprintf (stderr, "expand_opcode: illegal char '%c'\n", s[0]);
> ! 	  exit (1);
>   	case '0':
>   	case '1':
> ! 	    /* Consume an arbitrary number of ones and zeros.  */
> ! 	    do {
> ! 	      j = (j << 1) + (s[m++] - '0');
> ! 	    } while (s[m] == '0' || s[m] == '1');
> ! 	    expand_opcode ((val << m) | j, i, s + m);
>   	    break;

I think this is simpler:

    case '0':
    case '1':
        /* Consume an arbitrary number of ones and zeros.  */
        do {
          val = (val << 1) + (*s++ - '0');
        } while (s[m] == '0' || s[m] == '1');
        expand_opcode (val, i, s);
        break;

> !     case 'x':       /* xx -- 2-way fork */
> !       /* Cross-breeding with movy moved to separate function.  */
> !       for (j = 0; j < 4; j += 2)
> !         expand_opcode ((val << 2) | j, i, s + 2);
> !       break;
> !     case 'y':       /* yy -- two-way fork */
> !       for (j = 0; j < 2; j++)
> !         expand_opcode ((val << 2) | j, i, s + 2);
> !       break;
....
>       case 'X':
>       case 'a':
> !       /* XX, aa -- two-way fork */
> !       for (j = 0; j < 4; j += 2)
> !         expand_opcode ((val << 2) | j, i, s + 2);
>         break;
>       case 'Y':
>       case 'A':
> !       /* YY, AA -- two-way fork */
> !       for (j = 0; j < 2; j++)
> !         expand_opcode ((val << 2) | j, i, s + 2);
>         break;

You can do this with x0 / 0y / X0 / a0 / 0Y / 0A patterns now, have the '0'
be handled by the generic constant code, and thus only need a trivial case
of a one-letter, two-way fork.  gensim_caselist of course will need to have
the increments for these letters changed from two to one.

> --- 2108,2136 ----
>      processing insns (ppi) for code 0xf800 (ppi nopx nopy).  Copy the
>      latter tag to represent all combinations of ppi with ddt.  */
>   static void
> ! expand_ppi_movxy ()

You change what the function does; this should be reflected in the comment:

/* Table already contais all the switch case tags for separate movx and movy
   insns, and the switch case tag for processing parallel processing insns
   (ppi) for code 0xf800 (ppi nopx nopy).  Fill in all the movx/movy
   combinations, thus completing the coverage of the 16-bit opcode double
   data transfer (ddt) insns, and copy the ppi tag to represent all
   combinations of ppi with ddt.  */


> + 	    /* Ignore illegal nopy */
> + 	    if ((m & 7) == 0 && m != 0)
> + 	      continue;

The legal nopy has already been covered by expand_opcode, so we may skip it too.

	    /* Ignore non-movy.w patterns.  */
	    if ((m & 3) == 0 && m != 0)
	      continue;



About these new mov[xy].l instructions... do they actually have four opcodes each
that do the same thing?

Re: [RFA] sh-sim: restructure expand_opcode.

[Michael Snyder]

Joern Rennecke wrote:

> Yes, I agree that this patch improves the code.  Just a few more suggestions /
> corrections:
> 
> 
>>! /* Take an opcode, expand all varying fields in it out and fill all the
>>!    right entries in 'table' with the opcode index.  */
>>  
>>  static void
>>! expand_opcode (val, i, s)
> 
> 
> This never adhered to the GNU coding standards on comments (unless it preceded
> the clause on documenting arguments :-) .  This seems a good opportunity to
> fix this.  Something like:
> 
> /* Take an opcode, expand all varying fields in it out and fill all the
>    right entries in 'table' with the opcode index I.
>    VAL is the opcode computed so far, which needs left-shifting by the
>    number of bits that are still to be processed.  S is a string describing
>    the remaining bits.  */

OK, added.


> I think this is simpler:
> 
>     case '0':
>     case '1':
>         /* Consume an arbitrary number of ones and zeros.  */
>         do {
>           val = (val << 1) + (*s++ - '0');
>         } while (s[m] == '0' || s[m] == '1');
>         expand_opcode (val, i, s);
>         break;

No problem.

>>!     case 'x':       /* xx -- 2-way fork */
>>!       /* Cross-breeding with movy moved to separate function.  */
>>!       for (j = 0; j < 4; j += 2)
>>!         expand_opcode ((val << 2) | j, i, s + 2);
>>!       break;
>>!     case 'y':       /* yy -- two-way fork */
>>!       for (j = 0; j < 2; j++)
>>!         expand_opcode ((val << 2) | j, i, s + 2);
>>!       break;
> 
> ....
> 
>>      case 'X':
>>      case 'a':
>>!       /* XX, aa -- two-way fork */
>>!       for (j = 0; j < 4; j += 2)
>>!         expand_opcode ((val << 2) | j, i, s + 2);
>>        break;
>>      case 'Y':
>>      case 'A':
>>!       /* YY, AA -- two-way fork */
>>!       for (j = 0; j < 2; j++)
>>!         expand_opcode ((val << 2) | j, i, s + 2);
>>        break;
> 
> 
> You can do this with x0 / 0y / X0 / a0 / 0Y / 0A patterns now, have the '0'
> be handled by the generic constant code, and thus only need a trivial case
> of a one-letter, two-way fork.  gensim_caselist of course will need to have
> the increments for these letters changed from two to one.

Since I don't see any such patterns currently, let's save this
for a follow-on patch.  No use holding up perfectly good work
on the basis of a hypothetical future need.

> 
> 
>>--- 2108,2136 ----
>>     processing insns (ppi) for code 0xf800 (ppi nopx nopy).  Copy the
>>     latter tag to represent all combinations of ppi with ddt.  */
>>  static void
>>! expand_ppi_movxy ()
> 
> 
> You change what the function does; this should be reflected in the comment:
> 
> /* Table already contais all the switch case tags for separate movx and movy
>    insns, and the switch case tag for processing parallel processing insns
>    (ppi) for code 0xf800 (ppi nopx nopy).  Fill in all the movx/movy
>    combinations, thus completing the coverage of the 16-bit opcode double
>    data transfer (ddt) insns, and copy the ppi tag to represent all
>    combinations of ppi with ddt.  */

OK.



>>+ 	    /* Ignore illegal nopy */
>>+ 	    if ((m & 7) == 0 && m != 0)
>>+ 	      continue;
> 
> 
> The legal nopy has already been covered by expand_opcode, so we may skip it too.
> 
> 	    /* Ignore non-movy.w patterns.  */
> 	    if ((m & 3) == 0 && m != 0)
> 	      continue;
> 
> 
> 
> About these new mov[xy].l instructions... do they actually have four opcodes each
> that do the same thing?

Let's come back to this after the holidays.  Speaking of which,
by the time you reply, I will be gone for the holidays, and not
return until the new year.  If we have acheived convergence on
this piece of code, would you mind checking it in?

Cheers,
Michael

Re: [RFA] sh-sim: restructure expand_opcode.

[Joern Rennecke]

> Let's come back to this after the holidays.  Speaking of which,
> by the time you reply, I will be gone for the holidays, and not
> return until the new year.  If we have acheived convergence on
> this piece of code, would you mind checking it in?

patch complains about an out-of-sync patch, and I have no time this year to
investigate.
So I suppose the patch will need to wait for you to come back.  AFAI
understand the copyright provisions, if you want to add a copyright notice
for your patch (gencode.c currently says no copyright / provided by Cygnus),
that whould still be supposed to read 2003, as this is when the code was
written.

Hmmm... I wonder: since sim is considered to be part of gdb, does that mean
that blanket copyright assignments to the fsf covering gdb result(ed) in
any patches to gencode.c, which have been written by people working with
such an assignment in place, to be assigned to the FSF?  If that is the case,
a number of past patches would also need Copyright notices for FSF
Copyright.
We'll probably have to make up a new style (unless one to cover this
situation already exists), something like like: the version up to <date>
was placed by Cygnus into the public domain, while later modifications are
Copyright FSF (list of years).

Re: [RFA] sh-sim: restructure expand_opcode.

[Michael Snyder]

Joern Rennecke wrote:
>>Let's come back to this after the holidays.  Speaking of which,
>>by the time you reply, I will be gone for the holidays, and not
>>return until the new year.  If we have acheived convergence on
>>this piece of code, would you mind checking it in?
> 
> 
> patch complains about an out-of-sync patch, and I have no time this year to
> investigate.
> So I suppose the patch will need to wait for you to come back.  AFAI
> understand the copyright provisions, if you want to add a copyright notice
> for your patch (gencode.c currently says no copyright / provided by Cygnus),
> that whould still be supposed to read 2003, as this is when the code was
> written.

Committed.  We can return to the copyright issue.
I'll have the next patch ready shortly.

> 
> Hmmm... I wonder: since sim is considered to be part of gdb, does that mean
> that blanket copyright assignments to the fsf covering gdb result(ed) in
> any patches to gencode.c, which have been written by people working with
> such an assignment in place, to be assigned to the FSF?  If that is the case,
> a number of past patches would also need Copyright notices for FSF
> Copyright.
> We'll probably have to make up a new style (unless one to cover this
> situation already exists), something like like: the version up to <date>
> was placed by Cygnus into the public domain, while later modifications are
> Copyright FSF (list of years).
> 

[RFA] sh-sim: thislock/prevlock tweak

[Michael Snyder]

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


Joern,

I don't fully understand this code, but it looks to me as if this
minor change is needed.  Most other instructions appear to call
the macro "L()" for the register that was explicitly the target
of the instruction.

Michael


[-- Attachment #2: tmp --]
[-- Type: text/plain, Size: 1510 bytes --]

2004-02-05  Michael Snyder  <msnyder@redhat.com>

	* gencode.c (movua.l): Set thislock to 0, not n.

Index: gencode.c
===================================================================
RCS file: /cvs/src/src/sim/sh/gencode.c,v
retrieving revision 1.26
diff -p -r1.26 gencode.c
*** gencode.c	27 Jan 2004 23:30:01 -0000	1.26
--- gencode.c	5 Feb 2004 23:40:27 -0000
*************** op tab[] =
*** 869,875 ****
      "MA (1);",
      "R[0] = (RBAT (regn) << 24) + (RBAT (regn + 1) << 16) + ",
      "  (RBAT (regn + 2) << 8) + RBAT (regn + 3);",
!     "L (n);",
    },
    { "0n", "n", "movua.l @<REG_N>+,R0", "0100nnnn11101001",
      "int regn = R[n];",
--- 869,875 ----
      "MA (1);",
      "R[0] = (RBAT (regn) << 24) + (RBAT (regn + 1) << 16) + ",
      "  (RBAT (regn + 2) << 8) + RBAT (regn + 3);",
!     "L (0);",
    },
    { "0n", "n", "movua.l @<REG_N>+,R0", "0100nnnn11101001",
      "int regn = R[n];",
*************** op tab[] =
*** 877,883 ****
      "R[0] = (RBAT (regn) << 24) + (RBAT (regn + 1) << 16) + ",
      "  (RBAT (regn + 2) << 8) + RBAT (regn + 3);",
      "R[n] += 4;",
!     "L (n);",
    },
    { "", "mn", "mul.l <REG_M>,<REG_N>", "0000nnnnmmmm0111",
      "MACL = ((int) R[n]) * ((int) R[m]);",
--- 877,883 ----
      "R[0] = (RBAT (regn) << 24) + (RBAT (regn + 1) << 16) + ",
      "  (RBAT (regn + 2) << 8) + RBAT (regn + 3);",
      "R[n] += 4;",
!     "L (0);",
    },
    { "", "mn", "mul.l <REG_M>,<REG_N>", "0000nnnnmmmm0111",
      "MACL = ((int) R[n]) * ((int) R[m]);",

Re: [RFA] sh-sim: thislock/prevlock tweak

[Joern Rennecke]

> Joern,
> 
> I don't fully understand this code, but it looks to me as if this
> minor change is needed.  Most other instructions appear to call
> the macro "L()" for the register that was explicitly the target
> of the instruction.

Looks sensible as far as I can see it; it would be helpful to have
enough context to see the full insn.
Note that L is part of the mechanism that approximates SH[123]
timing; SH4 timing is entirely different.  This makes a number of
the MA calls rather bizarre, where we simulate the timings of a
processor that doesn't implement the instructions in the first place.

In October I made a patch to implement SH4 timings (controlled by
an #ifdef SH4_TIMINGS), but it ended up a few percent slower than
the simulator before, so I thought we should really use a simulator
built with ACE_FAST for the c-torture simulator tests - even better if we can
make it processor specific, i.e. no DSP insns for non-dsp processors
and no FPU insns for non-fpu processors.  The mere presence of the
code seems to skew the memory layout and/or register allocation of the
'hot' code to make the simulator slower.  So that would require to
build separate binaries and a wrapper that invokes the right one,
or make dejagnu pick up a specifically tuned variant.
Then we had a lot of changes to the simulator for other functionality,
and I didn't have the time to make this into a current patch.
I'll forward a copy of my letter with the patch in case you or someone
else on the list want to pick it up.

Re: [RFA] sh-sim: thislock/prevlock tweak

[Michael Snyder]

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

Joern Rennecke wrote:
>>Joern,
>>
>>I don't fully understand this code, but it looks to me as if this
>>minor change is needed.  Most other instructions appear to call
>>the macro "L()" for the register that was explicitly the target
>>of the instruction.
> 
> 
> Looks sensible as far as I can see it; it would be helpful to have
> enough context to see the full insn.

OK


[-- Attachment #2: tmp --]
[-- Type: text/plain, Size: 1699 bytes --]

Index: gencode.c
===================================================================
RCS file: /cvs/src/src/sim/sh/gencode.c,v
retrieving revision 1.26
diff -p -5 -r1.26 gencode.c
*** gencode.c	27 Jan 2004 23:30:01 -0000	1.26
--- gencode.c	6 Feb 2004 19:35:09 -0000
*************** op tab[] =
*** 867,885 ****
    { "0", "n", "movua.l @<REG_N>,R0", "0100nnnn10101001",
      "int regn = R[n];",
      "MA (1);",
      "R[0] = (RBAT (regn) << 24) + (RBAT (regn + 1) << 16) + ",
      "  (RBAT (regn + 2) << 8) + RBAT (regn + 3);",
!     "L (n);",
    },
    { "0n", "n", "movua.l @<REG_N>+,R0", "0100nnnn11101001",
      "int regn = R[n];",
      "MA (1);",
      "R[0] = (RBAT (regn) << 24) + (RBAT (regn + 1) << 16) + ",
      "  (RBAT (regn + 2) << 8) + RBAT (regn + 3);",
      "R[n] += 4;",
!     "L (n);",
    },
    { "", "mn", "mul.l <REG_M>,<REG_N>", "0000nnnnmmmm0111",
      "MACL = ((int) R[n]) * ((int) R[m]);",
    },
  #if 0  /* FIXME: The above cast to int is not really portable.
--- 867,885 ----
    { "0", "n", "movua.l @<REG_N>,R0", "0100nnnn10101001",
      "int regn = R[n];",
      "MA (1);",
      "R[0] = (RBAT (regn) << 24) + (RBAT (regn + 1) << 16) + ",
      "  (RBAT (regn + 2) << 8) + RBAT (regn + 3);",
!     "L (0);",
    },
    { "0n", "n", "movua.l @<REG_N>+,R0", "0100nnnn11101001",
      "int regn = R[n];",
      "MA (1);",
      "R[0] = (RBAT (regn) << 24) + (RBAT (regn + 1) << 16) + ",
      "  (RBAT (regn + 2) << 8) + RBAT (regn + 3);",
      "R[n] += 4;",
!     "L (0);",
    },
    { "", "mn", "mul.l <REG_M>,<REG_N>", "0000nnnnmmmm0111",
      "MACL = ((int) R[n]) * ((int) R[m]);",
    },
  #if 0  /* FIXME: The above cast to int is not really portable.

Re: [RFA] sh-sim: thislock/prevlock tweak

[Michael Snyder]

Michael Snyder wrote:
> Joern Rennecke wrote:
> 
>>> Joern,
>>>
>>> I don't fully understand this code, but it looks to me as if this
>>> minor change is needed.  Most other instructions appear to call
>>> the macro "L()" for the register that was explicitly the target
>>> of the instruction.
>>
>>
>>
>> Looks sensible as far as I can see it; it would be helpful to have
>> enough context to see the full insn.
> 
> 
> OK

Can I take this as approved?

> 
> 
> ------------------------------------------------------------------------
> 
> Index: gencode.c
> ===================================================================
> RCS file: /cvs/src/src/sim/sh/gencode.c,v
> retrieving revision 1.26
> diff -p -5 -r1.26 gencode.c
> *** gencode.c	27 Jan 2004 23:30:01 -0000	1.26
> --- gencode.c	6 Feb 2004 19:35:09 -0000
> *************** op tab[] =
> *** 867,885 ****
>     { "0", "n", "movua.l @<REG_N>,R0", "0100nnnn10101001",
>       "int regn = R[n];",
>       "MA (1);",
>       "R[0] = (RBAT (regn) << 24) + (RBAT (regn + 1) << 16) + ",
>       "  (RBAT (regn + 2) << 8) + RBAT (regn + 3);",
> !     "L (n);",
>     },
>     { "0n", "n", "movua.l @<REG_N>+,R0", "0100nnnn11101001",
>       "int regn = R[n];",
>       "MA (1);",
>       "R[0] = (RBAT (regn) << 24) + (RBAT (regn + 1) << 16) + ",
>       "  (RBAT (regn + 2) << 8) + RBAT (regn + 3);",
>       "R[n] += 4;",
> !     "L (n);",
>     },
>     { "", "mn", "mul.l <REG_M>,<REG_N>", "0000nnnnmmmm0111",
>       "MACL = ((int) R[n]) * ((int) R[m]);",
>     },
>   #if 0  /* FIXME: The above cast to int is not really portable.
> --- 867,885 ----
>     { "0", "n", "movua.l @<REG_N>,R0", "0100nnnn10101001",
>       "int regn = R[n];",
>       "MA (1);",
>       "R[0] = (RBAT (regn) << 24) + (RBAT (regn + 1) << 16) + ",
>       "  (RBAT (regn + 2) << 8) + RBAT (regn + 3);",
> !     "L (0);",
>     },
>     { "0n", "n", "movua.l @<REG_N>+,R0", "0100nnnn11101001",
>       "int regn = R[n];",
>       "MA (1);",
>       "R[0] = (RBAT (regn) << 24) + (RBAT (regn + 1) << 16) + ",
>       "  (RBAT (regn + 2) << 8) + RBAT (regn + 3);",
>       "R[n] += 4;",
> !     "L (0);",
>     },
>     { "", "mn", "mul.l <REG_M>,<REG_N>", "0000nnnnmmmm0111",
>       "MACL = ((int) R[n]) * ((int) R[m]);",
>     },
>   #if 0  /* FIXME: The above cast to int is not really portable.

Re: [RFA] sh-sim: thislock/prevlock tweak

[Joern Rennecke]

> Can I take this as approved?

I could check the timings, but then I couldn't make a decision regarding
the FSF repository because of the NDA.

So, let's say it's approved if you are satisfied that the timings make
more sense this way that without the patch.