This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Support VU0 on MIPS R5900


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, &regno);
> +		if (!file_mips_legacy_vu0 || mips_opts.arch != CPU_R5900)
> +		  ok = reg_lookup (&s, types, &regno);
> +		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, &regno))
>  		{
> +	    /* 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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]