Re: [RFA] Add sh4a to sh-sim (2nd iteration)

[Michael Snyder <msnyder at redhat dot com>]

Joern Rennecke wrote:
> > 	(expand_ppi_code): Flatten loop for simplicity, tweak for 12-bit
> > 	instead of 8-bit table (some insns are ambiguous to 8 bits).
>
>
> You are converting iteration into recursion - I don't see
> what is flat about deep recursion.

Just the code structure -- it's one "loop" shallower.
It's also more consistant with the other expand_opcode
function, in which all the recursions are explicit.

And it's not that deep -- upper limit, 12 levels;
in practice never that many.


> > --- gencode.c	7 Jan 2004 21:38:26 -0000
> > ***************
> > *** 1,4 ****
> > ! /* Simulator/Opcode generator for the Hitachi Super-H architecture.
> >  
> >     Written by Steve Chamberlain of Cygnus Support.
> >     sac@cygnus.com
> > --- 1,5 ----
> > ! /* Simulator/Opcode generator for the Renesas
> > !    (formerly Hitachi) Super-H architecture.
> >  
> >     Written by Steve Chamberlain of Cygnus Support.
> >     sac@cygnus.com
>
>
> That should be Renesas (formerly Hitachi) / SuperH Inc SuperH architecture.

OK.


> > +   /* sh4a */
> > +   { "", "", "ftrv <FV_N>", "1111vv0111111101",
> > +     "if (FPSCR_PR)",
> > +     "  RAISE_EXCEPTION (SIGILL);",
> > +     "else",
> > +     "{", 
> > +     "  /* FIXME not implemented.  */",
> > +     "  printf (\"ftrv xmtrx, FV%d\\n\", v1);",
> > +     "}", 
> > +   },
>
>
> ftrv is an sh4 instruction.

OK.


> >    { "n", "n", "ldc.l @<REG_N>+,DBR", "0100nnnn11110110",
> >      "MA (1);",
> >      "DBR = RLAT (R[n]);",
> >      "R[n] += 4;",
> >      "/* FIXME: user mode */",
> >    },
> > !   { "n", "n", "ldc.l @<REG_N>+,DBR", "0100nnnn11110110",
> > !     "if (SR_MD)",
> > !     "{ /* priv mode */",
> > !     "  MA (1);",
> > !     "  DBR = RLAT (R[n]);",
> > !     "  R[n] += 4;",
> > !     "}",
> > !     "else",
> > !     "  RAISE_EXCEPTION (SIGILL); /* user mode */",
> > !   },
>
>
> Why do you keep the old "ldc.l @<REG_N>+,DBR" version around?

Good question.  Assume that I will delete it, unles I say otherwise.


> > !   { "", "mn", "mul.l <REG_M>,<REG_N>", "0000nnnnmmmm0111",
> > !     "MACL = ((int) R[n]) * ((int) R[m]);",
> >    },
>
>
> The #if 0 was left there as a reminder that the casts to int are not fully
> portable.  The casts should be replaced with the SEXT32 macro.

OK, I'll either put them back or do the replacement.
If I put them back, I'll add this as a comment.


> > !   { "", "n", "movx.w @<REG_xy>,<DSP_XY>",   "111100xyXY0001??",
> >      "DSP_R (m) = RSWAT (R[n]) << 16;",
> > !     "if (iword & 3)",
> > !     "  {",
> > !     "    iword &= 0xfd53; goto top;",
> > !     "  }",
> > !   },
>
>
> I think I understand the way the new move instructions were added a bit
> better now.
> The implementation could be made faster by having the movx/nopy
> patterns separate, but I suppose speed is not such an issue for sh-dsp
> simulation, seeing that we don't have time-intensive testcases that
> use dsp move instructions.

Shall we commit it as is for now, and save speed improvements
for a later round?

> > *************** op ppi_tab[] =
> > *** 1379,1385 ****
> >      "COMPUTE_OVERFLOW;",
> >      "greater_equal = 0;",
> >    },
> > !   { "","", "pmuls Se,Sf,Dg",	"0100eeffxxyygguu",
> >      "res = (DSP_R (e) >> 16) * (DSP_R (f) >> 16) * 2;",
> >      "if (res == 0x80000000)",
> >      "  res = 0x7fffffff;",
> > --- 1581,1587 ----
> >      "COMPUTE_OVERFLOW;",
> >      "greater_equal = 0;",
> >    },
> > !   { "","", "pmuls Se,Sf,Dg",	"0100eeff0000gguu",
> >      "res = (DSP_R (e) >> 16) * (DSP_R (f) >> 16) * 2;",
> >      "if (res == 0x80000000)",
> >      "  res = 0x7fffffff;",
>
>
> According to the sh2-dsp manual that is still at the Renesas web site,
> the xx / yy fields are still present in the pmuls instruction.

Hmm, well, are they used for anything?  I think I took them out to
resolve a conflict with another insn (but I don't remember for sure).
Since there's no corresponding register parameter, and the code
does not use them -- is there any harm?  If the other patterns
are not used now, they probably will be someday.



> >    printf ("ppi_insn (iword)\n");
> >    printf ("     int iword;\n");
> >    printf ("{\n");
> > +   printf ("  /* 'ee' = [x0, x1, y0, a1] (FIXME [x0, x1, a1, m1]) */\n");
> >    printf ("  static char e_tab[] = { 8,  9, 10,  5};\n");
> > +   printf ("  /* 'ff' = [y0, y1, x0, a1] (FIXME [y0, y1, a1, m1]) */\n");
> >    printf ("  static char f_tab[] = {10, 11,  8,  5};\n");
> > +   printf ("  /* 'xx'(?) = [x0, x1, a0, a1]  */\n");
> >    printf ("  static char x_tab[] = { 8,  9,  7,  5};\n");
> > +   printf ("  /* 'yy'(?) = [y0, y1, m0, m1]  */\n");
> >    printf ("  static char y_tab[] = {10, 11, 12, 14};\n");
> > +   printf ("  /* 'gg' = [m0, m1, a0, a1]  */\n");
> >    printf ("  static char g_tab[] = {12, 14,  7,  5};\n");
> > +   printf ("  /* 'uu' = [x0, y0, a0, a1]  (FIXME [m1, x1, a0, a1]) */\n");
> >    printf ("  static char u_tab[] = { 8, 10,  7,  5};\n");
>
>
> What are these FIXMEs supposed to mean?

I did this work 4 months ago.  Probably  I thought the
comment was wrong, and that the actual set of registers
was as shown.  Your second opinion would be appreciated.