This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
RE: [PATCH] MIPS EVA ASE Support
- From: "Maciej W. Rozycki" <macro at codesourcery dot com>
- To: "Moore, Catherine" <Catherine_Moore at mentor dot com>
- Cc: Richard Sandiford <rdsandiford at googlemail dot com>, "binutils at sourceware dot org" <binutils at sourceware dot org>
- Date: Wed, 12 Jun 2013 17:51:28 +0100
- Subject: RE: [PATCH] MIPS EVA ASE Support
- References: <FD3DCEAC5B03E9408544A1E416F11242F8FC6EE7 at NA-MBX-01 dot mgc dot mentorg dot com> <87fvwz9hsg dot fsf at talisman dot default> <FD3DCEAC5B03E9408544A1E416F11242F8FC9639 at NA-MBX-01 dot mgc dot mentorg dot com> <87bo7g99yn dot fsf at talisman dot default> <FD3DCEAC5B03E9408544A1E416F11242F8FC9A3B at NA-MBX-01 dot mgc dot mentorg dot com> <87ip1l7oz9 dot fsf at talisman dot default> <FD3DCEAC5B03E9408544A1E416F11242F8FCA5F6 at NA-MBX-01 dot mgc dot mentorg dot com>
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