This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
RE: [MIPS] Add new virtualization instructions
- From: "Maciej W. Rozycki" <macro at codesourcery dot com>
- To: Chao-Ying Fu <Chao-Ying dot Fu at imgtec 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 12:26:53 +0100
- 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>
On Thu, 13 Jun 2013, Chao-Ying Fu wrote:
> Before the spec is posted, here is the patch to add microMIPS
> virtualization opcodes.
>
> 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.
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.
> 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...
> 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.
> 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]
I can't approve your patch, but FWIW it looks otherwise good to me.
Maciej