RE: [PATCH] MIPS SIMD Architecture (MSA) patch

[Chao-Ying Fu <Chao-Ying dot Fu at imgtec dot com>]

Maciej W. Rozycki wrote:
>  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?

  We will need probably two weeks to revise and publish the microMIPS MSA spec
to the imgtec website.  Please wait.
  
>  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.

  MSA and FP instructions can co-exist in a single binary.
Or, only MSA instructions exist in a single binary, theoretically.
Ex: We only use the integer SIMD instructions, and move data
between MSA registers and integer registers.
I want to keep the FP gnu attribute code as-is, and don't check
if the MSA gnu attribute may conflict with the FP gnu attribute.
My intention is that the only usage of MSA gnu attribute is to tell if this
binary uses 128-bit MSA.

> -- 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.

  Yes.  I will update my tests.

> 
> > 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.

  Yes.

> 
>  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.

  I haven't added branch relaxation support.  I think all possible MSA branches are there,
so it should be ok to support branch relaxation.  Need to work on it.

  Thanks a lot!

Regards,
Chao-ying