This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH] MIPS SIMD Architecture (MSA) patch
- From: Richard Sandiford <rdsandiford at googlemail dot com>
- To: Chao-Ying Fu <Chao-Ying dot Fu at imgtec 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:12:11 +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> <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>
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.
> @@ -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.
> @@ -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));
}
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)
...
> +/* 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;"
> @@ -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.)
> 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.
> 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".
> 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.
> @@ -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 { ... }".
> + /* 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.
> @@ -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 :-)
> @@ -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 ");"
Looks good otherwise, thanks.
Thanks,
Richard