[PATCH] MIPS EVA ASE Support

[Richard Sandiford]

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.

Thanks,
Richard