This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
RE: [PATCH] MIPS SIMD Architecture (MSA) patch
- From: Chao-Ying Fu <Chao-Ying dot Fu at imgtec dot com>
- To: 'Richard Sandiford' <rdsandiford at googlemail dot com>
- Cc: "'Maciej W. Rozycki'" <macro at codesourcery dot com>, "binutils at sourceware dot org" <binutils at sourceware dot org>
- Date: Thu, 10 Oct 2013 19:23:57 +0000
- Subject: RE: [PATCH] MIPS SIMD Architecture (MSA) patch
- Authentication-results: sourceware.org; auth=none
- References: <81D57523CB07B24881D63DE650C6ED82019034B4 at BADAG02 dot ba dot imgtec dot org> <87siwbr2os dot fsf at talisman dot default> <81D57523CB07B24881D63DE650C6ED82019039DC at BADAG02 dot ba dot imgtec dot org> <874n8rq66j dot fsf at talisman dot default> <81D57523CB07B24881D63DE650C6ED8201903F16 at BADAG02 dot ba dot imgtec dot org> <87y561ov0k dot fsf at talisman dot default>
Richard Sandiford wrote:
> Chao-Ying Fu <Chao-Ying.Fu@imgtec.com> writes:
> > @@ -14509,6 +14518,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_msa_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_msa_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_msa_bfd, ibfd,
> > + out_attr[Tag_GNU_MIPS_ABI_MSA].i,
> > + in_attr[Tag_GNU_MIPS_ABI_MSA].i);
> > + break;
> > + }
> > + }
> > + }
>
> 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.
> > @@ -1586,6 +1590,10 @@ static const struct mips_ase mips_ases[]
> >
> > { "virt", ASE_VIRT, ASE_VIRT64,
> > OPTION_VIRT, OPTION_NO_VIRT,
> > + 2, 2, 2, 2 },
> > +
> > + { "msa", ASE_MSA, ASE_MSA64,
> > + OPTION_MSA, OPTION_NO_MSA,
> > 2, 2, 2, 2 }
> > };
> >
>
> Just for the record, I suppose this is OK until we have r5 support.
Yes.
>
> > @@ -2567,6 +2576,40 @@ struct regname {
> > {"$ac2", RTYPE_ACC | 2}, \
> > {"$ac3", RTYPE_ACC | 3}
> >
> > +#define MSA_REGISTER_NAMES \
> > + {"$w0", RTYPE_MSA | 0}, \
> > + {"$w1", RTYPE_MSA | 1}, \
> > + {"$w2", RTYPE_MSA | 2}, \
> > + {"$w3", RTYPE_MSA | 3}, \
> > + {"$w4", RTYPE_MSA | 4}, \
> > + {"$w5", RTYPE_MSA | 5}, \
> > + {"$w6", RTYPE_MSA | 6}, \
> > + {"$w7", RTYPE_MSA | 7}, \
> > + {"$w8", RTYPE_MSA | 8}, \
> > + {"$w9", RTYPE_MSA | 9}, \
> > + {"$w10", RTYPE_MSA | 10}, \
> > + {"$w11", RTYPE_MSA | 11}, \
> > + {"$w12", RTYPE_MSA | 12}, \
> > + {"$w13", RTYPE_MSA | 13}, \
> > + {"$w14", RTYPE_MSA | 14}, \
> > + {"$w15", RTYPE_MSA | 15}, \
> > + {"$w16", RTYPE_MSA | 16}, \
> > + {"$w17", RTYPE_MSA | 17}, \
> > + {"$w18", RTYPE_MSA | 18}, \
> > + {"$w19", RTYPE_MSA | 19}, \
> > + {"$w20", RTYPE_MSA | 20}, \
> > + {"$w21", RTYPE_MSA | 21}, \
> > + {"$w22", RTYPE_MSA | 22}, \
> > + {"$w23", RTYPE_MSA | 23}, \
> > + {"$w24", RTYPE_MSA | 24}, \
> > + {"$w25", RTYPE_MSA | 25}, \
> > + {"$w26", RTYPE_MSA | 26}, \
> > + {"$w27", RTYPE_MSA | 27}, \
> > + {"$w28", RTYPE_MSA | 28}, \
> > + {"$w29", RTYPE_MSA | 29}, \
> > + {"$w30", RTYPE_MSA | 30}, \
> > + {"$w31", RTYPE_MSA | 31}
> > +
>
> Please instead generate this programmatically, in the loop:
>
> for (i = 0; i < 32; i++)
> {
> char regname[7];
>
> /* R5900 VU0 floating-point register. */
> regname[sizeof (rename) - 1] = 0;
> snprintf (regname, sizeof (regname) - 1, "$vf%d", i);
> symbol_table_insert (symbol_new (regname, reg_section,
> RTYPE_VF | i,
> &zero_address_frag));
>
> /* R5900 VU0 integer register. */
> snprintf (regname, sizeof (regname) - 1, "$vi%d", i);
> symbol_table_insert (symbol_new (regname, reg_section,
> RTYPE_VI | i,
> &zero_address_frag));
>
> }
Yes.
>
> I should do the same thing for the existing registers one day...
>
> > @@ -5146,6 +5203,48 @@ match_mdmx_imm_reg_operand (struct mips_
> > return TRUE;
> > }
> >
> > +/* OP_IMM_INDEX matcher. */
> > +
> > +static bfd_boolean
> > +match_imm_index_operand (struct mips_arg_info *arg,
> > + const struct mips_operand *operand)
> > +{
> > + int min_val, max_val;
> > + if (arg->token->type != OT_INTEGER_INDEX)
> > + return FALSE;
> > +
> > + min_val = 0;
> > + max_val = (1 << operand->size) - 1;
> > +
> > + if ((int) arg->token->u.index < min_val
> > + || (int) arg->token->u.index > max_val)
>
> We shouldn't cast to int like this, since it drops the high part of
> a user-supplied value. addressT is an unsigned type, so this
> should be:
>
> unsigned int max_val;
>
> if (arg->token->type != OT_INTEGER_INDEX)
> return FALSE;
>
> max_val = (1 << operand->size) - 1;
> if (arg->token->u.index > max_val)
> ...
Yes.
>
> > +/* OP_REG_INDEX matcher. */
> > +
> > +static bfd_boolean
> > +match_reg_index_operand (struct mips_arg_info *arg,
> > + const struct mips_operand *operand)
> > +{
> > + unsigned int regno;
> > + if (arg->token->type != OT_REG_INDEX)
> > + return FALSE;
> > +
> > + if (!match_regno (arg, OP_REG_GP, arg->token->u.regno, ®no))
> > + return FALSE;
> > +
> > + insn_insert_operand (arg->insn, operand, regno);
> > + ++arg->token;
> > + return TRUE;
> > +}
>
> Nit: blank line after "unsigned int regno;"
Yes.
>
> > @@ -16539,11 +16644,21 @@ md_convert_frag (bfd *abfd ATTRIBUTE_UNU
> > switch ((insn >> 28) & 0xf)
> > {
> > case 4:
> > - /* bc[0-3][tf]l? instructions can have the condition
> > - reversed by tweaking a single TF bit, and their
> > - opcodes all have 0x4???????. */
> > - gas_assert ((insn & 0xf3e00000) == 0x41000000);
> > - insn ^= 0x00010000;
> > + if ((insn & 0xff000000) == 0x47000000
> > + || (insn & 0xff600000) == 0x45600000)
> > + {
> > + /* bz.df/bnz.df, bz.v/bnz.v can have the condition
> > + reversed by tweaking bit 23. */
> > + insn ^= 0x00800000;
> > + }
> > + else
> > + {
> > + /* bc[0-3][tf]l? instructions can have
> the condition
> > + reversed by tweaking a single TF bit, and their
> > + opcodes all have 0x4???????. */
> > + gas_assert ((insn & 0xf3e00000) == 0x41000000);
> > + insn ^= 0x00010000;
> > + }
> > break;
> >
> > case 0:
> > @@ -16810,6 +16925,11 @@ md_convert_frag (bfd *abfd ATTRIBUTE_UNU
> > || (insn & 0xffe30000) == 0x42800000
> /* bc2f */
> > || (insn & 0xffe30000) == 0x42a00000)
> /* bc2t */
> > insn ^= 0x00200000;
> > + else if ((insn & 0xff000000) == 0x83000000 /* bz.df
> > +
> bnz.df */
> > + || (insn & 0xff600000) == 0x81600000) /* bz.v
> > +
> bnz.v */
> > + insn ^= 0x00800000;
> > else
> > abort ();
>
> Minor nit, sorry, but please use caps for the name in these two hunks,
> except for ".df", just as in the manual. (BZ.df, etc.). I
> was naively
> searching the opcode table for "bz.df" at first. (Again, I should go
> through and do the same for the existing code one day.)
Yes.
>
> > Index: gas/doc/c-mips.texi
> > ===================================================================
> > RCS file: /cvs/src/src/gas/doc/c-mips.texi,v
> > retrieving revision 1.80
> > diff -u -p -r1.80 c-mips.texi
> > --- gas/doc/c-mips.texi 12 Jul 2013 15:58:14 -0000 1.80
> > +++ gas/doc/c-mips.texi 9 Oct 2013 23:42:53 -0000
> > @@ -179,6 +179,12 @@ Generate code for the MCU Application Sp
> > This tells the assembler to accept MCU instructions.
> > @samp{-mno-mcu} turns off this option.
> >
> > +@item -mmsa
> > +@itemx -mno-msa
> > +Generate code for the MIPS SIMD Architecture Extension.
> > +This tells the assembler to accept MSA instructions.
> > +@samp{-mno-msa} turns off this option.
> > +
> > @item -mvirt
> > @itemx -mno-virt
> > Generate code for the Virtualization Application Specific
> Extension.
> > @@ -853,6 +859,14 @@ from the MCU Application Specific Extens
> > in the assembly. The @code{.set nomcu} directive prevents MCU
> > instructions from being accepted.
> >
> > +@cindex MIPS SIMD Architecture instruction generation override
> > +@kindex @code{.set msa}
> > +@kindex @code{.set nomsa}
> > +The directive @code{.set msa} makes the assembler accept
> instructions
> > +from the MIPS SIMD Architecture Extension from that point on
> > +in the assembly. The @code{.set nomsa} directive prevents MSA
> > +instructions from being accepted.
> > +
> > @cindex Virtualization instruction generation override
> > @kindex @code{.set virt}
> > @kindex @code{.set novirt}
>
> The options stuff is unfortunately duplicated in as.texinfo,
> so you need
> to update that too.
Yes.
>
> > Index: gas/testsuite/gas/mips/micromips@msa.s
> > ===================================================================
> > RCS file: gas/testsuite/gas/mips/micromips@msa.s
> > diff -N gas/testsuite/gas/mips/micromips@msa.s
>
> It looks like this is the same as msa.s except for the shifts in the
> branch offsets. It'd be good to have a single file is possible,
> with the shift amount controlled by a symbol. E.g. msa.d could have
> "--defsym insn_log2=2", micromips@msa.d could have "--defsym
> insn_log2=1"
> and the file could then use "<< insn_log2".
>
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.
>
> > @@ -1158,4 +1161,16 @@ enum
> > Val_GNU_MIPS_ABI_FP_64 = 4,
> > };
> >
> > +/* Object attribute values. */
> > +enum
> > +{
> > + /* Values defined for Tag_GNU_MIPS_ABI_MSA. */
> > +
> > + /* Not tagged or not using any ABIs affected by the
> differences. */
> > + Val_GNU_MIPS_ABI_MSA_ANY = 0,
> > +
> > + /* Using 128-bit MSA. */
> > + Val_GNU_MIPS_ABI_MSA_128 = 1,
> > +};
>
> It looks like the idea was that all new value definitions
> would be added to
> the existing "enum { ... }".
Yes.
>
> > + /* MSA registers $w0-$w31. */
> > + OP_REG_MSA,
> > +
> > + /* MSA control registers. */
> > + OP_REG_MSA_CTRL
>
> "MSA control registers $0-$31", to make the syntax more
> obvious at a glance.
Yes.
>
> > @@ -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. :-)
> > @@ -1250,6 +1280,16 @@ print_insn_arg (struct disassemble_info
> > case OP_VU0_MATCH_SUFFIX:
> > print_vu0_channel (info, operand, uval);
> > break;
> > +
> > + case OP_IMM_INDEX:
> > + infprintf (is, "[%d]", uval);
> > + break;
> > +
> > + case OP_REG_INDEX:
> > + infprintf (is, "[" );
> > + print_reg (info, opcode, OP_REG_GP, uval);
> > + infprintf (is, "]" );
> > + break;
> > }
>
> Excess psaces before ");"
Yes.
>
> Looks good otherwise, thanks.
>
Thanks a lot for your quick review! I will upload a new version soon.
Regards,
Chao-ying