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

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

Richard Sandiford wrote:
> Chao-Ying Fu <Chao-Ying.Fu@imgtec.com> writes:
> >> I wonder whether -mmsa is the best name for the 128-bit 
> option if we
> >> know that a 256-bit form might come along.
> >
> >   I am ok with any names.  Or, we can add another switch to 
> specify the
> > width of MSA,
> > if 256-bit MSA comes along.
> 
> OK.  I assume that if a new revision of the ASE comes along 
> with 256-bit
> support, it'll still try to be compatible with the original revision?

  Yes, I think 256-bit MSA will try to be compatible to 128-bit MSA.
The same MSA instruction will operate on 256-bit MSA registers.
Hopefully, old code for 128-bit MSA will still run with no or a little bit helps.

> If so, we could end up needing to specify the width and the revision
> separately.

  Yes.

> 
> So maybe it would be better for GCC to have a -mmsa128 option 
> to specify
> the register width.  This would be like -mgp32/-mgp64 and 
> -mfp32/-mfp64.
> Then plain -mmsa can be used to specify the revision, a bit like -mdsp
> and -mdspr2.  (Similarly for ".set dsp(r2)" and ".set msa".)

  Yes.

> 
> The assembler would only need to know about -mmsa, so there'd 
> be no changes
> to the option handling in the rest of the patch.  It's just a 
> question of
> what the GCC option will be called, and what should be used 
> in these error
> messages.

  Yes.

> 
> > > > Index: include/elf/mips.h
> > > > 
> ===================================================================
> > > > RCS file: /cvs/src/src/include/elf/mips.h,v
> > > > retrieving revision 1.55
> > > > diff -u -p -r1.55 mips.h
> > > > --- include/elf/mips.h	17 Sep 2013 21:05:49 -0000	1.55
> > > > +++ include/elf/mips.h	9 Oct 2013 23:42:58 -0000
> > > > @@ -1135,6 +1135,9 @@ enum
> > > >  
> > > >    /* Floating-point ABI used by this object file.  */
> > > >    Tag_GNU_MIPS_ABI_FP = 4,
> > > > +
> > > > +  /* MSA ABI used by this object file.  */
> > > > +  Tag_GNU_MIPS_ABI_MSA = 8,
> > > >  };
> > > 
> > > These tags are enums rather than bitfields.  Are 5-7 
> already taken?
> > > If not, and if this isn't "in the wild" yet, it'd be good to use 5
> > > instead if possible.
> >
> >   The selection of 8 is due to the attr types designed by 
> bfd/elf-attrs.c.
> > Ex:
> > /* Determine whether a GNU object attribute tag takes an integer, a
> >    string or both.  */
> > static int
> > gnu_obj_attrs_arg_type (int tag)
> > {
> >   /* Except for Tag_compatibility, for GNU attributes we follow the
> >      same rule ARM ones > 32 follow: odd-numbered tags take strings
> >      and even-numbered tags take integers.  In addition, tag & 2 is
> >      nonzero for architecture-independent tags and zero for
> >      architecture-dependent ones.  */
> >   if (tag == Tag_compatibility)
> >     return 3;
> >   else
> >     return (tag & 1) != 0 ? 2 : 1;
> > }
> >
> > Similarly, "include/elf/ppc.h" uses 4, and 8, and 12 for 
> object attribute tags.
> 
> Doh, hadn't realised that, sorry.
> 
> > > > @@ -513,7 +522,7 @@ const struct mips_arch_choice mips_arch_
> > > >    { "mips64r2",	1, bfd_mach_mipsisa64r2, CPU_MIPS64R2,
> > > >      ISA_MIPS64R2,
> > > >      (ASE_MIPS3D | ASE_DSP | ASE_DSPR2 | ASE_DSP64 | 
> ASE_EVA | ASE_MT
> > > > -     | ASE_MDMX | ASE_MCU | ASE_VIRT | ASE_VIRT64),
> > > > +     | ASE_MDMX | ASE_MCU | ASE_VIRT | ASE_VIRT64 | 
> ASE_MSA | ASE_MSA64),
> > > >      mips_cp0_names_mips3264r2,
> > > >      mips_cp0sel_names_mips3264r2, ARRAY_SIZE 
> (mips_cp0sel_names_mips3264r2),
> > > >      mips_hwr_names_mips3264r2 },
> > > > @@ -738,6 +747,18 @@ parse_mips_dis_option (const char *optio
> > > >        return;
> > > >      }
> > > >  
> > > > +  if (CONST_STRNEQ (option, "msa"))
> > > > +    {
> > > > +      mips_ase |= ASE_MSA;
> > > > +      if ((mips_isa & INSN_ISA_MASK) == ISA_MIPS64R2)
> > > > +	{
> > > > +	  mips_ase |= ASE_MSA64;
> > > > +	  /* Disable ASE_MDMX, because of opcode conflicts.  */
> > > > +	  mips_ase &= ~ASE_MDMX;
> > > > +	}
> > > > +      return;
> > > > +    }
> > > 
> > > Hmm, but you keep MDMX in the default list for mips64r2.  
> > > Maybe we really
> > > do need to add mips64r5 :-)
> >
> >   Sure.  New work to do.  :-)
> 
> Still, adding an incompatible ASE to mips64r2 just seems wrong.
> We should either remove ASE_MDMX or leave ASE_MSA* out (of both
> the mips32r2 and mips64r2 entries).  Since the only in-tree processor
> with MDMX support is the MIPS 64r1 SB1, I'm happy with dropping MDMX
> if that's easier.  There might be some testsuite fallout from doing
> that though.

  I dropped ASE_MDMX from mips64r2 and the testing result is ok.
Only the MIPS 64r1 SB1 tests MDMX.
If we do add mips32r5 and mips64r5, we will drop ASE_MSA* from mips32r2 and mips64r2 then.

Ex: (mips-dis.c)
@@ -513,7 +522,7 @@ const struct mips_arch_choice mips_arch_
   { "mips64r2",        1, bfd_mach_mipsisa64r2, CPU_MIPS64R2,
     ISA_MIPS64R2,
     (ASE_MIPS3D | ASE_DSP | ASE_DSPR2 | ASE_DSP64 | ASE_EVA | ASE_MT
-     | ASE_MDMX | ASE_MCU | ASE_VIRT | ASE_VIRT64),
+     | ASE_MCU | ASE_VIRT | ASE_VIRT64 | ASE_MSA | ASE_MSA64),
     mips_cp0_names_mips3264r2,
     mips_cp0sel_names_mips3264r2, ARRAY_SIZE (mips_cp0sel_names_mips3264r2),
     mips_hwr_names_mips3264r2 },
@@ -738,6 +747,14 @@ parse_mips_dis_option (const char *optio
       return;
     }

+  if (CONST_STRNEQ (option, "msa"))
+    {
+      mips_ase |= ASE_MSA;
+      if ((mips_isa & INSN_ISA_MASK) == ISA_MIPS64R2)
+         mips_ase |= ASE_MSA64;
+      return;
+    }
+
   if (CONST_STRNEQ (option, "virt"))
     {
       mips_ase |= ASE_VIRT;

> 
> OK with those changes, thanks,
> 
> Richard
> 

  I will start to check in.  Thanks a lot!

Regards,
Chao-ying