This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH] Support VU0 on MIPS R5900
- From: Richard Sandiford <rdsandiford at googlemail dot com>
- To: JÃrgen Urban <JuergenUrban at gmx dot de>
- Cc: binutils at sourceware dot org
- Date: Tue, 29 Jan 2013 20:56:50 +0000
- Subject: Re: [PATCH] Support VU0 on MIPS R5900
- References: <20130108234130.27410@gmx.net>
Hi JÃrgen,
I'm really sorry for the slow response. The patch mostly looks good,
but I've got a few questions:
> + /* RTYPE_VF registers have optional appended suffixes
> + for R5900 VU instructions. */
> + if (mips_opts.arch == CPU_R5900 && (types & RTYPE_VF))
> + {
> + while(!ISDIGIT(*(e-1)) && e != *s)
> + --e;
> + }
> +
Is there any need for this mips_opts.arch test? It looked on face
value like the "types & RTYPE_VF" part was right on its own.
> insn = (struct mips_opcode *) hash_find (hash, name);
>
> - if (insn != NULL || !mips_opts.micromips)
> - break;
> + if (insn != NULL
> + || (!mips_opts.micromips && mips_opts.arch != CPU_R5900))
> + break;
> if (forced_insn_length)
> - break;
> + break;
> + if (tried_suffix)
> + {
> + if(dot)
> + *dot = '.';
> + tried_suffix = 0;
> + break;
> + }
> + /* Used for VU0 insn.xyz, insn.xyzw, or any other variation.
> + The suffix is needed for comparing with operand suffixes. */
> + if (mips_opts.arch == CPU_R5900)
> + {
> + dot = strrchr (name,'.');
> + opend = dot != NULL ? dot - name : end;
> + if (opend < 3)
> + break;
> + if (dot)
> + {
> + *dot = '\0';
> + dot++;
> + insn_suffix = alloca (strlen(dot)+1);
> + strncpy(insn_suffix,dot,strlen(dot));
> + insn_suffix[strlen(dot)] = '\0';
> + dot--;
> + }
> + tried_suffix = 1;
> + }
"forced_insn_length" has no meaning for the standard encoding, so I
think it should remain in the micromips-specific part. The VU-specific
stuff should happen regardless of mips_opts.arch.
Why "opend < 3"? It looked like that had been copied from the
micromips code. I don't think the length of the bit before the "."
really matters for VU0.
The code allows any instruction to have a suffix (including those
without "+j"), so it needs to check that insn_suffix has been consumed.
That might need a few control-flow changes, since mips_ip has some
early non-error exits.
FWIW, I think the patch below might help to make the VU stuff easier to add.
> @@ -11269,7 +11420,17 @@ mips_ip (char *str, struct mips_cl_insn
> || (mips_opts.micromips
> && args[1] == 'm'
> && (args[2] == 'l' || args[2] == 'n'
> - || args[2] == 's' || args[2] == 'a')));
> + || args[2] == 's' || args[2] == 'a'))
> + || (mips_opts.arch == CPU_R5900
> + && args[1] == '+' && args[2] == '9')
> + || (mips_opts.arch == CPU_R5900
> + && args[1] == '+' && args[2] == '-'
> + && args[3] == '+'
> + && (args[4] == '0' || args[4] == '9'))
> + || (mips_opts.arch == CPU_R5900
> + && args[1] == '+'
> + && (args[2] == '0' || args[2] == '9')
> + && args[3] == '+' && args[4] == '+'));
> if (*s == '\0' && args[1] == 'b')
> return;
> /* Fall through. */
I don't think we should be checking "mips_opts.arch == CPU_R5900" here,
especially since this is an assert about a static property of the table.
We check elsewhere whether or not the instruction belongs to the
current ISA.
> + case 'f': /* 15 bit immediate in bit 6 for vcallms */
> + my_getExpression (&imm_expr, s);
> + check_absolute_expr (ip, &imm_expr);
> + min_range = 0;
> + max_range = 0xff8;
> + if (imm_expr.X_add_number < min_range ||
> + imm_expr.X_add_number > max_range)
> + {
> + as_bad (_("Micromem offset not in range 0x%lx..0x%lx (0x%lx)"),
> + (long) min_range, (long) max_range,
> + (long) imm_expr.X_add_number);
> + }
> + if (imm_expr.X_add_number & 0x7)
> + as_bad (_("Micromem offset is not aligned 0x%lx"),
> + (long)imm_expr.X_add_number);
> + /* Not in specs, division is done for simplicity */
> + INSERT_OPERAND (0, VCALLMS, *ip, (imm_expr.X_add_number >> 3));
> + imm_expr.X_op = O_absent;
> + s = expr_end;
> + continue;
I don't understand the comment, sorry.
> + if (!file_mips_legacy_vu0)
> + {
> + if (strncmp(disreg_name,s,2)
> + || (s[2] != '\0' && s[2] != ',' && !ISSPACE(s[2])))
> + as_bad (_("Expected `%s' register"),disreg_name);
> + }
> + else
> + {
> + if (disreg_name[1] != s[0]
> + || (s[1] != '\0' && s[1] != ',' && !ISSPACE(s[1])))
> + as_bad (_("Expected `%c' register"),disreg_name[1]);
> + }
> + }
> + while(*s != '\0' && *s != ',' && !ISSPACE(*s))
> + s++;
Would is_part_of_name work instead of these s[2] and s[1] tests?
Instead of the while loop, it seems better for the "if" statements
to skip over the name. E.g. something like:
if (!file_mips_legacy_vu0)
{
if (strncmp (disreg_name, s, 2) == 0
&& !is_part_of_name (s[2]))
s += 2;
else
as_bad (_("Expected `%s' register"), disreg_name);
}
else
{
if (disreg_name[1] == s[0]
&& !is_part_of_name (s[1]))
s += 1;
else
as_bad (_("Expected `%c' register"), disreg_name[1]);
}
> + for (i = 0; i < strlen(insn_suffix); i++)
> + if (insn_suffix[i] == valid[0])
> + fields[0]++;
> + else if (insn_suffix[i] == valid[1])
> + fields[1]++;
> + else if (insn_suffix[i] == valid[2])
> + fields[2]++;
> + else if (insn_suffix[i] == valid[3])
> + fields[3]++;
> + else
> + as_bad (_("Invalid character in opcode suffix "
> + "`%c'"), insn_suffix[i]);
for (i = 0; insn_suffix[i]; i++)
> + case 'k': /* Optional R5900 VU operand suffix */
> + {
> + unsigned int length = 0;
> + s_reset = s;
> + if (*s == '\0' || ISSPACE(*s) || *s == ',')
> + continue;
> + while (*s != '\0' && !ISSPACE(*s) && *s != ',')
> + {
> + length++;
> + s++;
> + }
> + s = s_reset;
> + if (insn_suffix)
> + if (strncmp(s,insn_suffix,strlen(insn_suffix))
> + || length != strlen(insn_suffix))
> + insn_error ="Mismatched operand suffix";
> + s += length;
> + }
> + continue;
This seems to allow any suffix in the case where insn_suffix is null
(which AIUI is equivalent to ".xyzw"). Is that intentional, or should
we match "xyzw" in that case?
All errors need to be wrapped in _(...) so that they get picked up for
translation.
> @@ -11922,7 +12353,34 @@ mips_ip (char *str, struct mips_cl_insn
> cop0 = opcode == OP_OP_COP0;
> }
> types = RTYPE_NUM | (cop0 ? RTYPE_CP0 : RTYPE_GP);
> - ok = reg_lookup (&s, types, ®no);
> + if (!file_mips_legacy_vu0 || mips_opts.arch != CPU_R5900)
> + ok = reg_lookup (&s, types, ®no);
> + else
> + {
> + if (*s == '$')
> + ++s;
> + if (!strncmp(s,"vf",2))
> + s+=2;
> + else if (!strncmp(s,"vi",2))
> + s+=2;
> +
> + if (ISDIGIT(*s))
> + {
> + regno = 0;
> + do
> + {
> + regno *= 10;
> + regno += *s - '0';
> + ++s;
> + }
> + while (ISDIGIT (*s));
> +
> + if (regno > 31)
> + ok = 0;
> + else
> + ok = 1;
> + }
> + }
> if (mips_opts.micromips)
> INSERT_OPERAND (1, RS, *ip, regno);
> else
Here too mips_opts.arch != CPU_R5900 seems the wrong thing to check.
We should be checking a property of the instruction instead.
Please factor out the file_mips_legacy_vu0 parsing into a separate function,
since it appears three times.
> @@ -12130,6 +12615,8 @@ mips_ip (char *str, struct mips_cl_insn
> s_reset = s;
> if (reg_lookup (&s, rtype, ®no))
> {
> + /* Jumped from extended (+ 5,6,7,8,9,0) args for R5900 */
> + vu0:
> if ((regno & 1) != 0
> && HAVE_32BIT_FPRS
> && !mips_oddfpreg_ok (ip->insn_mo, argnum))
Please try to avoid the long goto. Which parts of:
if ((regno & 1) != 0
&& HAVE_32BIT_FPRS
&& !mips_oddfpreg_ok (ip->insn_mo, argnum))
as_warn (_("Float register should be even, was %d"),
regno);
c = *args;
if (*s == ' ')
++s;
if (args[1] != *s)
{
if (c == 'V' || c == 'W')
{
regno = lastregno;
s = s_reset;
++args;
}
}
apply in the VU0 case?
> @@ -19556,7 +20053,8 @@ MIPS options:\n\
> -msoft-float do not allow floating-point instructions\n\
> -msingle-float only allow 32-bit floating-point operations\n\
> -mdouble-float allow 32-bit and 64-bit floating-point operations\n\
> ---[no-]construct-floats [dis]allow floating point values to be constructed\n"
> +--[no-]construct-floats [dis]allow floating point values to be constructed\n\
> +-mr5900-legacy-vu0 allow legacy VU0 register names\n"
> ));
> #ifdef OBJ_ELF
> fprintf (stream, _("\
This needs to be documented in doc/c-mips.texi.
> + case 'd': /* FTF field for r5900 VU floating point register */
> + case 'e': /* FSF field for r5900 VU floating point register */
> + case 'l': /* BC field for r5900 VU instructions */
> + {
> + unsigned int field = 0;
> +
> + if (*d == 'd')
> + field = GET_OP (l, VFTF);
> + if (*d == 'e')
> + field = GET_OP (l, VFSF);
> + if (*d == 'l')
> + field = GET_OP (l, VBC);
> +
> + if (field == 0)
> + infprintf (is, "x");
> + if (field == 1)
> + infprintf (is, "y");
> + if (field == 2)
> + infprintf (is, "z");
> + if (field == 3)
> + infprintf (is, "w");
> + }
> + break;
> +
Please split the "field == ..." stuff out into a separate function,
or simply use:
infprintf (is, "%c", "xyzw"[GET_OP (l, ...)]);
three times, one after each case statement.
There are some places where the patch doesn't follow the coding conventions
(usually spaces missing before "(" and ","), but I'm happy to fix those
up before committing.
Thanks,
Richard
gas/
* config/tc-mips.c (micromips_strip_length): New function,
split out from...
(mips_ip): ...here.
Index: gas/config/tc-mips.c
===================================================================
--- gas/config/tc-mips.c 2013-01-26 16:42:59.000000000 +0000
+++ gas/config/tc-mips.c 2013-01-29 19:58:34.644152580 +0000
@@ -10768,6 +10768,33 @@ expr_const_in_range (expressionS *ep, of
&& ep->X_add_number < max << bit);
}
+/* Remove any micromips length suffix from instruction mnemonic NAME
+ (of length LENGTH) and store it in forced_insn_length. Return true
+ if something changed. */
+
+static bfd_boolean
+micromips_strip_length (char *name, int length)
+{
+ char *dot;
+ long opend;
+
+ /* The length goes before any "." suffix. */
+ dot = strchr (name, '.');
+ opend = dot != NULL ? dot - name : length;
+ if (opend < 3)
+ return FALSE;
+
+ if (name[opend - 2] == '1' && name[opend - 1] == '6')
+ forced_insn_length = 2;
+ else if (name[opend - 2] == '3' && name[opend - 1] == '2')
+ forced_insn_length = 4;
+ else
+ return FALSE;
+
+ memcpy (name + opend - 2, name + opend, length - opend + 1);
+ return TRUE;
+}
+
/* This routine assembles an instruction into its binary format. As a
side effect, it sets one of the global variables imm_reloc or
offset_reloc to the type of relocation to do if one of the operands
@@ -10793,11 +10820,9 @@ mips_ip (char *str, struct mips_cl_insn
unsigned int limlo, limhi;
char *s_reset;
offsetT min_range, max_range;
- long opend;
char *name;
int argnum;
unsigned int rtype;
- char *dot;
long end;
insn_error = NULL;
@@ -10824,35 +10849,16 @@ mips_ip (char *str, struct mips_cl_insn
memcpy (name, str, end);
name[end] = '\0';
- for (;;)
- {
- insn = (struct mips_opcode *) hash_find (hash, name);
-
- if (insn != NULL || !mips_opts.micromips)
- break;
- if (forced_insn_length)
- break;
-
- /* See if there's an instruction size override suffix,
- either `16' or `32', at the end of the mnemonic proper,
- that defines the operation, i.e. before the first `.'
- character if any. Strip it and retry. */
- dot = strchr (name, '.');
- opend = dot != NULL ? dot - name : end;
- if (opend < 3)
- break;
- if (name[opend - 2] == '1' && name[opend - 1] == '6')
- forced_insn_length = 2;
- else if (name[opend - 2] == '3' && name[opend - 1] == '2')
- forced_insn_length = 4;
- else
- break;
- memcpy (name + opend - 2, name + opend, end - opend + 1);
- }
- if (insn == NULL)
+ insn = (struct mips_opcode *) hash_find (hash, name);
+ if (!insn)
{
- insn_error = _("Unrecognized opcode");
- return;
+ if (mips_opts.micromips && micromips_strip_length (name, end))
+ insn = (struct mips_opcode *) hash_find (hash, name);
+ if (!insn)
+ {
+ insn_error = _("Unrecognized opcode");
+ return;
+ }
}
/* For microMIPS instructions placed in a fixed-length branch delay slot