[PATCH] MIPS EVA ASE Support
Moore, Catherine
Catherine_Moore@mentor.com
Wed Jun 12 15:48:00 GMT 2013
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: eva.cl3
Type: application/octet-stream
Size: 1949 bytes
Desc: eva.cl3
URL: <https://sourceware.org/pipermail/binutils/attachments/20130612/670a0a88/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: eva.patch3
Type: application/octet-stream
Size: 174130 bytes
Desc: eva.patch3
URL: <https://sourceware.org/pipermail/binutils/attachments/20130612/670a0a88/attachment-0001.obj>
More information about the Binutils
mailing list