Home Original page

[PATCH] MIPS SIMD Architecture (MSA) patch

Maciej W. Rozycki macro@codesourcery.com
Tue Oct 8 17:20:00 GMT 2013 
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
More information about the Binutils mailing list