This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH] MIPS SIMD Architecture (MSA) patch
- From: "Maciej W. Rozycki" <macro at codesourcery dot com>
- To: Chao-Ying Fu <Chao-Ying dot Fu at imgtec dot com>
- Cc: "binutils at sourceware dot org" <binutils at sourceware dot org>
- Date: Tue, 8 Oct 2013 18:20:12 +0100
- Subject: Re: [PATCH] MIPS SIMD Architecture (MSA) patch
- Authentication-results: sourceware.org; auth=none
- References: <81D57523CB07B24881D63DE650C6ED82019034B4 at BADAG02 dot ba dot imgtec dot org>
Hi Chao-ying,
Thanks for your contribution. Here are a few notes addressing some small
issues I've noticed as I skimmed over your proposal; please do not
consider them though to be a full review that Richard will undoubtedly be
happy to follow with.
> We would like to contribute a patch to support MIPS SIMD Architectures (MSA).
> The MSA spec (version 1.07) can be found from http://www.imgtec.com/mips/mips-simd.asp.
Do you plan to release microMIPS instruction encoding documentation as
well?
> Index: bfd/elfxx-mips.c
> ===================================================================
> RCS file: /cvs/src/src/bfd/elfxx-mips.c,v
> retrieving revision 1.360
> diff -u -p -r1.360 elfxx-mips.c
> --- bfd/elfxx-mips.c 24 Sep 2013 22:15:37 -0000 1.360
> +++ bfd/elfxx-mips.c 8 Oct 2013 00:29:51 -0000
> @@ -14317,7 +14317,9 @@ mips_elf_merge_obj_attributes (bfd *ibfd
>
> abi_fp_bfd = mips_elf_tdata (obfd)->abi_fp_bfd;
> in_attr = elf_known_obj_attributes (ibfd)[OBJ_ATTR_GNU];
> - if (!abi_fp_bfd && in_attr[Tag_GNU_MIPS_ABI_FP].i != Val_GNU_MIPS_ABI_FP_ANY)
> + if (!abi_fp_bfd
> + && (in_attr[Tag_GNU_MIPS_ABI_FP].i != Val_GNU_MIPS_ABI_FP_ANY
> + || in_attr[Tag_GNU_MIPS_ABI_MSA].i != Val_GNU_MIPS_ABI_MSA_ANY))
> mips_elf_tdata (obfd)->abi_fp_bfd = ibfd;
>
> if (!elf_known_obj_attributes_proc (obfd)[0].i)
> @@ -14509,6 +14511,47 @@ mips_elf_merge_obj_attributes (bfd *ibfd
> }
> }
>
> + /* Check for conflicting Tag_GNU_MIPS_ABI_MSA attributes and merge
> + non-conflicting ones. */
> + if (in_attr[Tag_GNU_MIPS_ABI_MSA].i != out_attr[Tag_GNU_MIPS_ABI_MSA].i)
> + {
> + out_attr[Tag_GNU_MIPS_ABI_MSA].type = 1;
> + if (out_attr[Tag_GNU_MIPS_ABI_MSA].i == Val_GNU_MIPS_ABI_MSA_ANY)
> + out_attr[Tag_GNU_MIPS_ABI_MSA].i = in_attr[Tag_GNU_MIPS_ABI_MSA].i;
> + else if (in_attr[Tag_GNU_MIPS_ABI_MSA].i != Val_GNU_MIPS_ABI_MSA_ANY)
> + switch (out_attr[Tag_GNU_MIPS_ABI_MSA].i)
> + {
> + case Val_GNU_MIPS_ABI_MSA_128:
> + _bfd_error_handler
> + (_("Warning: %B uses %s (set by %B), "
> + "%B uses unknown MSA ABI %d"),
> + obfd, abi_fp_bfd, ibfd,
> + "-mmsa", in_attr[Tag_GNU_MIPS_ABI_MSA].i);
> + break;
> +
> + default:
> + switch (in_attr[Tag_GNU_MIPS_ABI_MSA].i)
> + {
> + case Val_GNU_MIPS_ABI_MSA_128:
> + _bfd_error_handler
> + (_("Warning: %B uses unknown MSA ABI %d "
> + "(set by %B), %B uses %s"),
> + obfd, abi_fp_bfd, ibfd,
> + out_attr[Tag_GNU_MIPS_ABI_MSA].i, "-mmsa");
> + break;
> +
> + default:
> + _bfd_error_handler
> + (_("Warning: %B uses unknown MSA ABI %d "
> + "(set by %B), %B uses unknown MSA ABI %d"),
> + obfd, abi_fp_bfd, ibfd,
> + out_attr[Tag_GNU_MIPS_ABI_MSA].i,
> + in_attr[Tag_GNU_MIPS_ABI_MSA].i);
> + break;
> + }
> + }
> + }
> +
> /* Merge Tag_compatibility attributes and any common GNU ones. */
> _bfd_elf_merge_object_attributes (ibfd, obfd);
>
Is the use of MSA and FP operations in a single binary mutually
exclusive? If so, then you need to check for conflicting use. If not,
then you need to use a separate variable/struct member to remember and
report the first significant MSA BFD; abi_msa_bfd seems the likely name
candidate.
> Index: gas/testsuite/gas/mips/micromips@msa.d
> ===================================================================
> RCS file: gas/testsuite/gas/mips/micromips@msa.d
> diff -N gas/testsuite/gas/mips/micromips@msa.d
> --- /dev/null 1 Jan 1970 00:00:00 -0000
> +++ gas/testsuite/gas/mips/micromips@msa.d 8 Oct 2013 00:29:57 -0000
> @@ -0,0 +1,787 @@
> +#objdump: -dr --prefix-addresses --show-raw-insn -Mmsa
> +#name: MSA instructions
> +#as: -32 -mmsa
> +
> +.*: +file format .*mips.*
> +
> +Disassembly of section \.text:
> +[0-9a-f]+ <[^>]*> 5802 081a sll.b \$w0,\$w1,\$w2
> +[0-9a-f]+ <[^>]*> 5825 20da sll.h \$w3,\$w4,\$w5
-- etc. I suggest that you match the dot in instruction mnemonics
literally, i.e.:
+[0-9a-f]+ <[^>]*> 5802 081a sll\.b \$w0,\$w1,\$w2
+[0-9a-f]+ <[^>]*> 5825 20da sll\.h \$w3,\$w4,\$w5
and likewise throughout the remaining test cases. I haven't checked if we
have any other mnemonics that could match your original wildcard patterns,
but I think it won't hurt to be strict here.
> Index: opcodes/mips-dis.c
> ===================================================================
> RCS file: /cvs/src/src/opcodes/mips-dis.c,v
> retrieving revision 1.116
> diff -u -p -r1.116 mips-dis.c
> --- opcodes/mips-dis.c 19 Aug 2013 18:56:59 -0000 1.116
> +++ opcodes/mips-dis.c 8 Oct 2013 00:30:27 -0000
> @@ -738,10 +747,22 @@ parse_mips_dis_option (const char *optio
[...]
> if (CONST_STRNEQ (option, "virt"))
> {
> mips_ase |= ASE_VIRT;
> - if (mips_isa & ISA_MIPS64R2)
> + if ((mips_isa & INSN_ISA_MASK) == ISA_MIPS64R2)
> mips_ase |= ASE_VIRT64;
> return;
> }
This looks like an unrelated bug fix to me that should be applied
separately.
Also I've noticed the MSA module adds new branch instructions -- have you
wired them into our branch relaxation support (GAS's --relax-branch
option) -- if it's at all possible? Assuming that it is, then it's OK, at
least initially, as far as I am concerned, if you did not, but either way
please add some test coverage, following either relax.? or relax-bc1any.?
from gas/testsuite/gas/mips/ as applicable.
Maciej