This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
RE: [MIPS] Add new virtualization instructions
- From: Chao-Ying Fu <Chao-Ying dot Fu at imgtec dot com>
- To: "'Maciej W. Rozycki'" <macro at codesourcery dot com>
- Cc: "'Pinski, Andrew'" <Andrew dot Pinski at caviumnetworks dot com>, 'Richard Sandiford' <rdsandiford at googlemail dot com>, "binutils at sourceware dot org" <binutils at sourceware dot org>
- Date: Thu, 13 Jun 2013 17:40:03 +0000
- Subject: RE: [MIPS] Add new virtualization instructions
- References: <e790fa722ba34ff097a964d1dfa06e9d at BY2PR07MB058 dot namprd07 dot prod dot outlook dot com> <87wqr81o3b dot fsf at sandifor-thinkpad dot stglab dot manchester dot uk dot ibm dot com> <98cba9069a7141f399456267fe33b95f at BY2PR07MB058 dot namprd07 dot prod dot outlook dot com> <87d2szptbr dot fsf at sandifor-thinkpad dot stglab dot manchester dot uk dot ibm dot com>,<81D57523CB07B24881D63DE650C6ED82EDBB9B at bamail02 dot ba dot imgtec dot org> <bd8d0ebef46949869722652cd2cab2d1 at BY2PR07MB058 dot namprd07 dot prod dot outlook dot com> <81D57523CB07B24881D63DE650C6ED82018D3FE3 at BADAG02 dot ba dot imgtec dot org> <alpine dot DEB dot 1 dot 10 dot 1306130105100 dot 16287 at tp dot orcam dot me dot uk>
Maciej W. Rozycki wrote:
> > gas/ChangLog
> > 2013-06-12 Chao-ying Fu <Chao-ying.Fu@imgtec.com>
> >
> > * config/tc-mips.c (ISA_SUPPORTS_VIRT_ASE): Support micromips.
> >
> > gas/testsuite/ChangLog
> > 2013-06-12 Chao-ying Fu <Chao-ying.Fu@imgtec.com>
> >
> > * gas/mips/micromips@virt.d: New file.
> > gas/mips/micromips@virt64.d: New file.
> > gas/mips/mips.exp: Change micromips to use
> -march=mips64r2, as the virt64 test needs it.
>
> Why do you need to change it? It's proved useful in finding
> bugs -- ASEs
> that should be made available to microMIPS assembly
> unconditionally such
> as with the EVA support discussed as recently as yesterday.
If I add -mips64r2 to the as flag for the micromips@virt64.d test,
I will get this error.
Ex: micromips@virt64.d
#objdump: -dr --prefix-addresses --show-raw-insn -Mvirt,cp0-names=mips64r2
#name: virt64 instructions
#source: virt64.s
#as: -64 -mvirt -mips64r2
....
Ex: Testing
../as-new -64 -mvirt -mips64r2 -march=mips64 -mmicromips -o dump.o /home/fu/dev
/gcc-mainline/src/gas/testsuite/gas/mips/virt64.s
Executing on host: sh -c {../as-new -64 -mvirt -mips64r2 -march=mips64 -mmicrom
ips -o dump.o /home/fu/dev/gcc-mainline/src/gas/testsuite/gas/mips/virt64.s 2>&1
} /dev/null gas.out (timeout = 300)
Assembler messages:
Error: -mips64r2 conflicts with the other architecture options, which imply -mip
s64
/home/fu/dev/gcc-mainline/src/gas/testsuite/gas/mips/virt64.s:5: Error: Opcode n
ot supported on this processor: mips64 (mips64) `dmfgc0 $3,$29'
....
Or, if I change ISA_SUPPORTS_VIRT64_ASE to support micromips with MIPS64,
I won't need the mips64r2 change in mips.exp and the micromips virt64 test will run.
Ex:
Index: config/tc-mips.c
===================================================================
RCS file: /cvs/src/src/gas/config/tc-mips.c,v
retrieving revision 1.538
diff -u -p -r1.538 tc-mips.c
--- config/tc-mips.c 10 Jun 2013 18:15:47 -0000 1.538
+++ config/tc-mips.c 13 Jun 2013 17:37:10 -0000
@@ -381,9 +381,12 @@ static int file_ase_mt;
static int file_ase_virt;
#define ISA_SUPPORTS_VIRT_ASE (mips_opts.isa == ISA_MIPS32R2 \
- || mips_opts.isa == ISA_MIPS64R2)
+ || mips_opts.isa == ISA_MIPS64R2 \
+ || mips_opts.micromips)
-#define ISA_SUPPORTS_VIRT64_ASE (mips_opts.isa == ISA_MIPS64R2)
+#define ISA_SUPPORTS_VIRT64_ASE (mips_opts.isa == ISA_MIPS64R2 \
+ || (mips_opts.isa == ISA_MIPS64 \
+ && mips_opts.micromips))
/* The argument of the -march= flag. The architecture we are assembling. */
static int file_mips_arch = CPU_UNKNOWN;
Are there other solutions to let the micromips virt64 test run?
>
> Please keep the formatting of ChangLog entries correct, not
> exceeding 79
> columns and preferably staying within 74, and with an
> asterisk preceding
> file names. Also these entries are meant not to document the
> purpose of
> any changes made, just the changes themselves (please provide
> any reasons
> for individual changes you think are worth noting within the
> explanation
> provided along the patch submission though).
>
> E.g.:
>
> * gas/mips/micromips@virt.d: New file.
> * gas/mips/micromips@virt64.d: New file.
> * gas/mips/mips.exp: Change micromips to use -march=mips64r2.
Yes.
> > Index: gas/testsuite/gas/mips/micromips@virt.d
> > ===================================================================
> > RCS file: gas/testsuite/gas/mips/micromips@virt.d
> > diff -N gas/testsuite/gas/mips/micromips@virt.d
> > --- /dev/null 1 Jan 1970 00:00:00 -0000
> > +++ gas/testsuite/gas/mips/micromips@virt.d 12 Jun 2013
> 23:10:06 -0000
> > @@ -0,0 +1,21 @@
> > +#objdump: -dr --prefix-addresses --show-raw-insn
> -Mvirt,cp0-names=mips32r2
> > +#name: virt instructions
> > +#source: virt.s
> > +#as: -32 -mvirt
> > +
> > +.*: +file format .*mips.*
> > +
> > +Disassembly of section \.text:
> > +[0-9a-f]+ <[^>]*> 007d 04fc mfgc0 v1,c0_taghi
> > +[0-9a-f]+ <[^>]*> 0174 2cfc mfgc0 t3,\$20,5
> > +[0-9a-f]+ <[^>]*> 02e2 06fc mtgc0 s7,c0_entrylo0
> > +[0-9a-f]+ <[^>]*> 00ee 16fc mtgc0 a3,\$14,2
> > +[0-9a-f]+ <[^>]*> 0000 c37c hypcall
> > +[0-9a-f]+ <[^>]*> 0256 c37c hypcall 0x256
> > +[0-9a-f]+ <[^>]*> 0000 417c tlbginv
> > +[0-9a-f]+ <[^>]*> 0000 517c tlbginvf
> > +[0-9a-f]+ <[^>]*> 0000 017c tlbgp
> > +[0-9a-f]+ <[^>]*> 0000 117c tlbgr
> > +[0-9a-f]+ <[^>]*> 0000 217c tlbgwi
> > +[0-9a-f]+ <[^>]*> 0000 317c tlbgwr
> > + ...
>
> Minor nit -- please use \.\.\. here...
Yes.
>
> > Index: gas/testsuite/gas/mips/micromips@virt64.d
> > ===================================================================
> > RCS file: gas/testsuite/gas/mips/micromips@virt64.d
> > diff -N gas/testsuite/gas/mips/micromips@virt64.d
> > --- /dev/null 1 Jan 1970 00:00:00 -0000
> > +++ gas/testsuite/gas/mips/micromips@virt64.d 12 Jun
> 2013 23:10:06 -0000
> > @@ -0,0 +1,13 @@
> > +#objdump: -dr --prefix-addresses --show-raw-insn
> -Mvirt,cp0-names=mips64r2
> > +#name: virt64 instructions
> > +#source: virt64.s
> > +#as: -64 -mvirt
> > +
> > +.*: +file format .*mips.*
> > +
> > +Disassembly of section \.text:
> > +[0-9a-f]+ <[^>]*> 587d 00e7 dmfgc0 v1,c0_taghi
> > +[0-9a-f]+ <[^>]*> 5974 28e7 dmfgc0 a7,\$20,5
> > +[0-9a-f]+ <[^>]*> 5ae2 02e7 dmtgc0 s7,c0_entrylo0
> > +[0-9a-f]+ <[^>]*> 58ee 12e7 dmtgc0 a3,\$14,2
> > + ...
>
> ... and here.
Yes.
>
> > Index: gas/testsuite/gas/mips/mips.exp
> > ===================================================================
> > RCS file: /cvs/src/src/gas/testsuite/gas/mips/mips.exp,v
> > retrieving revision 1.220
> > diff -u -p -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 23:10:06 -0000
> > @@ -442,7 +442,7 @@ mips_arch_create mips64r2 64 mips64 { mi
> > mips_arch_create mips16 32 {} {} \
> > { -march=mips1 -mips16 } { -mmips:16 }
> > mips_arch_create micromips 64 mips64r2 {} \
> > - { -march=mips64 -mmicromips } {}
> > + { -march=mips64r2 -mmicromips } {}
> > mips_arch_create r3000 32 mips1 {} \
> > { -march=r3000 -mtune=r3000 } { -mmips:3000 }
> > mips_arch_create r3900 32 mips1 { gpr_ilocks } \
> > @@ -791,8 +791,8 @@ if { [istarget mips*-*-vxworks*] } {
> > run_dump_test "lineno"
> > run_dump_test "sync"
> >
> > - run_dump_test_arches "virt" [mips_arch_list_matching
> mips32r2 !micromips]
> > - run_dump_test_arches "virt64" [mips_arch_list_matching
> mips64r2 !micromips]
> > + run_dump_test_arches "virt" [mips_arch_list_matching mips32r2]
> > + run_dump_test_arches "virt64" [mips_arch_list_matching
> mips64r2]
>
> Please use a tab after the test names as in the following two tests:
>
> >
> > run_dump_test_arches "mips32" [mips_arch_list_matching mips32]
> > run_dump_test_arches "mips32-imm"
> [mips_arch_list_matching mips32]
Yes.
>
> I can't approve your patch, but FWIW it looks otherwise good to me.
>
> Maciej
>
Thanks for your review!
Regards,
Chao-ying