This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

RE: [PATCH] MIPS EVA ASE Support


On Wed, 12 Jun 2013, Moore, Catherine wrote:

> 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?

 The cause is in tc-mips.c, see below.  Please see a couple of minor nits 
too.

> Index: gas/config/tc-mips.c
> ===================================================================
> RCS file: /cvs/src/src/gas/config/tc-mips.c,v
> retrieving revision 1.538
> diff -p -u -r1.538 tc-mips.c
> --- gas/config/tc-mips.c	10 Jun 2013 18:15:47 -0000	1.538
> +++ gas/config/tc-mips.c	12 Jun 2013 15:34:58 -0000
> @@ -292,7 +293,8 @@ static int file_mips_single_float = 0;
> static struct mips_set_options mips_opts =
> {
>    /* isa */ ISA_UNKNOWN, /* ase_mips3d */ -1, /* ase_mdmx */ -1,
> -  /* ase_smartmips */ 0, /* ase_dsp */ -1, /* ase_dspr2 */ -1, /* ase_mt */ -1,
> +  /* ase_smartmips */ 0, /* ase_dsp */ -1, /* ase_dspr2 */ -1, 

 Trailing whitespace here.

> +  /* ase_eva */ -1, /* ase_mt */ -1,
>    /* ase_mcu */ -1, /* ase_virt */ -1, /* mips16 */ -1,/* micromips */ -1,
>    /* noreorder */ 0,  /* at */ ATREG, /* warn_about_macros */ 0,
>    /* nomove */ 0, /* nobopt */ 0, /* noautoextend */ 0, /* gp32 */ 0,

 While fixing this perhaps it would be a good idea to reformat the next 
two lines too, i.e.:

  /* ase_eva */ -1, /* ase_mt */ -1, /* ase_mcu */ -1, /* ase_virt */ -1,
  /* mips16 */ -1, /* micromips */ -1,

(notice the fixed missing space before micromips too)?

> @@ -365,6 +367,13 @@ static int file_ase_dspr2;
> 			        || mips_opts.isa == ISA_MIPS64R2	\
> 				|| mips_opts.micromips)
>  
> +/* True if -meva was passed or implied by arguments passed on the
> +   command line (e.g., by -march).  */
> +static int file_ase_eva;
> +
> +#define ISA_SUPPORTS_EVA_ASE (mips_opts.isa == ISA_MIPS32R2		\
> +			      || mips_opts.isa == ISA_MIPS64R2)
> +

 Here's the cause of the test failure, just use what e.g. 
ISA_SUPPORTS_DSPR2_ASE is defined to immediately above.

> @@ -8519,21 +8676,21 @@ macro (struct mips_cl_insn *ip)
> 			     "t,r,j", tempreg, breg, BFD_RELOC_LO16);
> 	      macro_build (NULL, s, fmt, treg, tempreg);
> 	    }
> -	  else if (!off12)
> +	  else if (offbits == 16)
> 	    macro_build (&offset_expr, s, fmt, treg, BFD_RELOC_LO16, breg);
> 	  else
> 	    macro_build (NULL, s, fmt,
> 			 treg, (unsigned long) offset_expr.X_add_number, breg);
> 	}
> -      else if (off12 || off0)
> +      else if (off0 || offbits != 16)

 How about making off0 a part of the offbits scheme too?

> 	{
> -	  /* A 12-bit or 0-bit offset field is too narrow to be used
> -	     for a low-part relocation, so load the whole address into
> -	     the auxillary register.  In the case of "A(b)" addresses,
> +	  /* A 12-bit, 0-bit or 9-bit offset field is too narrow to be

 Suggest sorting the bit count, i.e.:

	  /* A 12-bit, 9-bit or 0-bit offset field is too narrow to be

> Index: gas/testsuite/gas/mips/mips.exp
> ===================================================================
> RCS file: /cvs/src/src/gas/testsuite/gas/mips/mips.exp,v
> retrieving revision 1.220
> diff -p -u -r1.220 mips.exp
> --- gas/testsuite/gas/mips/mips.exp	10 Jun 2013 18:15:48 -0000	1.220
> +++ gas/testsuite/gas/mips/mips.exp	12 Jun 2013 15:34:58 -0000
> @@ -428,14 +428,14 @@ mips_arch_create mips5 	64	mips4	{ fpisa
>  mips_arch_create mips32	32	mips2	{} \
> 			{ -march=mips32 -mtune=mips32 } { -mmips:isa32 } \
> 			{ mipsisa32-*-* mipsisa32el-*-* }
> -mips_arch_create mips32r2 32	mips32	{ fpisa3 fpisa4 fpisa5 ror } \
> +mips_arch_create mips32r2 32	mips32	{ fpisa3 fpisa4 fpisa5 eva ror } \
> 			{ -march=mips32r2 -mtune=mips32r2 } \
> 			{ -mmips:isa32r2 } \
> 			{ mipsisa32r2-*-* mipsisa32r2el-*-* }
>  mips_arch_create mips64	64	mips5	{ mips32 } \
> 			{ -march=mips64 -mtune=mips64 } { -mmips:isa64 } \
> 			{ mipsisa64-*-* mipsisa64el-*-* }
> -mips_arch_create mips64r2 64	mips64	{ mips32r2 ror } \
> +mips_arch_create mips64r2 64	mips64	{ mips32r2 eva ror } \
> 			{ -march=mips64r2 -mtune=mips64r2 } \
> 			{ -mmips:isa64r2 } \
> 			{ mipsisa64r2-*-* mipsisa64r2el-*-* }

 Please document the "eva" flag within this file too (next to "ror" 
documentation).  However is there a need for this flag in the first place?  
It looks like an architecture property to me, the "mips32r2" qualifier 
should do...

> @@ -849,6 +849,8 @@ if { [istarget mips*-*-vxworks*] } {
> 					[mips_arch_list_matching mips64r2 \
> 					    !micromips]
>  
> +    run_dump_test_arches "eva"		[mips_arch_list_matching eva !octeon]
> +

 ... i.e.:

    run_dump_test_arches "eva"		[mips_arch_list_matching mips32r2 \
					    !octeon]

> Index: opcodes/micromips-opc.c
> ===================================================================
> RCS file: /cvs/src/src/opcodes/micromips-opc.c,v
> retrieving revision 1.10
> diff -p -u -r1.10 micromips-opc.c
> --- opcodes/micromips-opc.c	8 Jun 2013 10:22:55 -0000	1.10
> +++ opcodes/micromips-opc.c	12 Jun 2013 15:34:58 -0000
> @@ -110,6 +110,10 @@
> /* MIPS MCU (MicroController) ASE support.  */
> #define MC	ASE_MCU
>  
> +/* MIPS Enhanced VA Scheme.  */
> +#define EVA     ASE_EVA

 Spaces instead of a tab here.

> +#define TLBINV	ASE_EVA

 I think this should be separate from EVA and documented, e.g.:

/* TLB invalidate instruction support.  */
#define TLBINV	ASE_EVA

(that follows the CP0.Config4.IE definition).

> Index: opcodes/mips-dis.c
> ===================================================================
> RCS file: /cvs/src/src/opcodes/mips-dis.c,v
> retrieving revision 1.101
> diff -p -u -r1.101 mips-dis.c
> --- opcodes/mips-dis.c	8 Jun 2013 10:22:55 -0000	1.101
> +++ opcodes/mips-dis.c	12 Jun 2013 15:34:58 -0000
> @@ -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 0. */
> +		      delta = GET_OP_S (insn, EVAOFFSET);
> +		      if (delta & 0x100)
> +			delta |= ~MICROMIPSOP_MASK_EVAOFFSET;

 No need for the conditional statement now that GET_OP_S does the 
necessary bit juggling.

> Index: opcodes/mips-opc.c
> ===================================================================
> RCS file: /cvs/src/src/opcodes/mips-opc.c,v
> retrieving revision 1.98
> diff -p -u -r1.98 mips-opc.c
> --- opcodes/mips-opc.c	8 Jun 2013 10:22:55 -0000	1.98
> +++ opcodes/mips-opc.c	12 Jun 2013 15:34:58 -0000
> @@ -194,6 +194,10 @@
> /* MIPS MCU (MicroController) ASE support.  */
> #define MC	ASE_MCU
> 
> +/* MIPS Enhanced VA Scheme.  */
> +#define EVA	ASE_EVA
> +#define TLBINV	ASE_EVA

 Same issue about TLBINV as in micromips-opc.c.

 I'll let Richard comment on the rest.

  Maciej


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]