RE: [PATCH] MIPS EVA ASE Support

["Moore, Catherine" <Catherine_Moore at mentor dot com>]

Hi Richard,

> -----Original Message-----
> From: Richard Sandiford [mailto:rdsandiford@googlemail.com]
> Sent: Monday, June 10, 2013 3:20 PM
> 
> Generally looks good, just a few minor comments...
> 
> "Moore, Catherine" <Catherine_Moore@mentor.com> writes:
> > @@ -6454,6 +6475,7 @@ macro (struct mips_cl_insn *ip)
> >    int likely = 0;
> >    int coproc = 0;
> >    int off12 = 0;
> > +  int off9 = 0;
> 
> Please instead fold these two variables into a single one.  E.g.:
> 
>   int offbits = 16;
> 
> > +		    /* If this value won't fit into the offset, then go find
> > +		       a macro that will generate a 16- or 32-bit offset code
> > +		       pattern.  */
> > +		    i = my_getSmallExpression (&imm_expr, imm_reloc, s);
> > +		    if ((i == 0 && (imm_expr.X_op != O_constant
> > +			 || imm_expr.X_add_number >= 1 << shift
> > +			 || imm_expr.X_add_number < -1 << shift))
> 
> Formatting, should be:
> 
> 		    if ((i == 0 && (imm_expr.X_op != O_constant
> 				    || imm_expr.X_add_number >= 1 << shift
> 				    || imm_expr.X_add_number < -1 << shift))
> 
> > @@ -15427,6 +15609,12 @@ mips_after_parse_args (void)
> >      as_warn (_("%s ISA does not support DSP R2 ASE"),
> >  	     mips_cpu_info_from_isa (mips_opts.isa)->name);
> >
> > +  if (mips_opts.ase_eva == -1)
> > +      mips_opts.ase_eva = (arch_info->flags & MIPS_CPU_ASE_EVA) ? 1 :
> > + 0;
> 
> There should only be two spaces of indentation.
> 
> > +  else if (strcmp (name, "noeva") == 0)
> > +    {
> > +      mips_opts.ase_eva = 0;
> > +    }
> 
> Redundant braces.
> 
> > Index: gas/testsuite/gas/mips/eva.d
> >
> ==========================================================
> =========
> > RCS file: gas/testsuite/gas/mips/eva.d diff -N
> > gas/testsuite/gas/mips/eva.d
> > --- /dev/null	1 Jan 1970 00:00:00 -0000
> > +++ gas/testsuite/gas/mips/eva.d	10 Jun 2013 18:16:12 -0000
> > @@ -0,0 +1,1410 @@
> > +#objdump: -d --prefix-addresses --show-raw-insn
> 
> Since this test includes symbolic addresses, it'd be better to use -dr rather
> than -d, so that we can check the relocations used.
> 
> The test above has all the addresses stubbed out (good), but the microMIPS
> one seems to have a mixture:
> 
> > +0+0198 <[^>]*> 0121 0950 	addu	at,at,t1
> > +0+019c <[^>]*> 6101 6200 	lhue	t0,0\(at\)
> > +0+01a0 <1a0> 3020 fe00 	li	at,-512
> > +0+01a4 <1a4> 6141 6200 	lhue	t2,0\(at\)
> > +0+01a8 <1a8> 3020 0200 	li	at,512
> 
> Unless there's a particular reason, these should be stubbed out too.
> 
> > Index: include/opcode/mips.h
> >
> ==========================================================
> =========
> > RCS file: /cvs/src/src/include/opcode/mips.h,v
> > retrieving revision 1.89
> > diff -p -u -r1.89 mips.h
> > --- include/opcode/mips.h	8 Jun 2013 10:22:55 -0000	1.89
> > +++ include/opcode/mips.h	10 Jun 2013 18:16:12 -0000
> > @@ -330,6 +330,10 @@
> >  #define OP_MASK_IMMY		0
> >  #define OP_SH_IMMY		0
> >
> > +/* MIPS32 Enhanced VA Scheme */
> > +#define OP_SH_EVAOFFSET		7
> > +#define OP_MASK_EVAOFFSET	0x1ff
> 
> As with the GCC patch, I'd prefer not to hard-code MIPS32.  It could easily be
> extended to MIPS64 in future.
> 
> (Other instances too, including the microMIPS cases.)
> 
> > @@ -774,6 +778,8 @@ static const unsigned int mips_isa_table
> >  #define ASE_DSP64		0x00000002
> >  /* DSP R2 ASE  */
> >  #define ASE_DSPR2		0x00000004
> > +/* MIPS32 Enhanced VA Scheme */
> > +#define ASE_EVA			0x00000080
> >  /* MCU (MicroController) ASE */
> >  #define ASE_MCU			0x00000010
> >  /* MDMX ASE */
> 
> Should be 0x08 rather than 0x80.  0x80 is ASE_MT.
> 
> > +{"tlbinv",  "",		0x0000437c, 0xffffffff,	INSN_TLB,
> 	0,		I1	},
> > +{"tlbinvf", "",		0x0000537c, 0xffffffff,	INSN_TLB,
> 	0,		I1	},
> 
> Is the idea really to allow these for general microMIPS, even when EVA isn't
> selected?  There should be a test for it so, and a comment here saying why.
> 
> > @@ -1152,6 +1152,10 @@ print_insn_args (const char *d,
> >  	      infprintf (is, "%s", mips_fpr_names[GET_OP (l, FZ)]);
> >  	      break;
> >
> > +	    case 'j':	/* 9-bit signed offset in bit 6.  */
> > +	      infprintf (is, "%d", GET_OP_S (l, EVAOFFSET));
> > +	      break;
> 
> bit 7
> 
> > @@ -2655,6 +2659,13 @@ print_insn_micromips (bfd_vma memaddr, s
> >  		      infprintf (is, "0x%x", msbd + 1);
> >  		      break;
> >
> > +		    case 'j':   /* 9-bit signed offset in bit 6 */
> > +		      delta = GET_OP (insn, EVAOFFSET);
> > +		      if (delta & 0x100)
> > +			delta |= ~MICROMIPSOP_MASK_EVAOFFSET;
> > +		      infprintf (is, "%d", delta);
> > +		      break;
> 
> bit 0.  Should use GET_OP_S here too.
> 
> > @@ -1640,6 +1643,8 @@ const struct mips_opcode mips_builtin_op
> >  {"tgeu",    "s,t,q",	0x00000031, 0xfc00003f, RD_s|RD_t|TRAP,
> 	0,		I2	},
> >  {"tgeu",    "s,j",	0x04090000, 0xfc1f0000, RD_s|TRAP,		0,
> 		I2	}, /* tgeiu */
> >  {"tgeu",    "s,I",	0,    (int) M_TGEU_I,	INSN_MACRO,		0,
> 		I2	},
> > +{"tlbinv",  "",         0x42000003, 0xffffffff, INSN_TLB,       	0,
> 	I33   	},
> > +{"tlbinvf", "",         0x42000004, 0xffffffff, INSN_TLB,       	0,
> 	I33   	},
> 
> Same point/question as above.
> 
I've now made the edits as you suggested.  I also defined TLBINV and used it in the opcode table as you and others suggested.  During the course of updating the testsuite to check the relocs, I discovered that the microMIPS variant of the EVA test was not being run.  I tried to fix this in a number of ways, but was not successful.  This version of the patch updates the arches list in mips.exp to include eva for mips64r2.  This causes -march=mips64 to be run against the eva test.  The EVA ASE isn't supported for mips64;  it's limited to mips64r2.  What is the correct way to change the arches table and/or the testing options to reflect that limitation? 

Thanks,
Catherine

Attachment: eva.cl3
Description: eva.cl3

Attachment: eva.patch3
Description: eva.patch3