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

[Richard Sandiford <rdsandiford at googlemail dot com>]

Thanks for the patch.  Looks good at first glance, although I agree with
Maciej's comments.  One thing that stood out though:

Chao-Ying Fu <Chao-Ying.Fu@imgtec.com> writes:
> +  /* MSA register element $w0[0]-$w31[31].  $w is at bit 11.
> +     Index is at bit 16.  */
> +  OP_MSA_ELEM0_31,
> +
> +  /* MSA register element $w0[0]-$w31[15].  $w is at bit 11.
> +     Index is at bit 16.  */
> +  OP_MSA_ELEM0_15,
> +
> +  /* MSA register element $w0[0]-$w31[7].  $w is at bit 11.
> +     Index is at bit 16.  */
> +  OP_MSA_ELEM0_7,
> +
> +  /* MSA register element $w0[0]-$w31[3].  $w is at bit 11.
> +     Index is at bit 16.  */
> +  OP_MSA_ELEM0_3,
> +
> +  /* MSA register element $w0[0]-$w31[0].  $w is at bit 11.  Index is 0.  */
> +  OP_MSA_ELEM0,
> +
> +  /* MSA register element $w0[$0]-$w31[$31].  $w is at bit 11.
> +     Index register is at bit 16.  */
> +  OP_MSA_ELEM_REG,
> +
> +  /* MSA register element $w0[0]-$w31[31].  $w is at bit 6.
> +     Index is at bit 16.  */
> +  OP_MSA2_ELEM0_31,
> +
> +  /* MSA register element $w0[0]-$w31[15].  $w is at bit 6.
> +     Index is at bit 16.  */
> +  OP_MSA2_ELEM0_15,
> +
> +  /* MSA register element $w0[0]-$w31[7].  $w is at bit 6.
> +     Index is at bit 16.  */
> +  OP_MSA2_ELEM0_7,
> +
> +  /* MSA register element $w0[0]-$w31[3].  $w is at bit 6.
> +     Index is at bit 16.  */
> +  OP_MSA2_ELEM0_3

The idea was to try to treat individual fields as individual operand
types where possible.  MDMX was an exception because the two 5-bit
fields effectively formed a single 10-bit field that specified a
full register, a broadcast element, or a constant.  I.e. the fields
together could have three different forms.

Here the operand types seem to have a single form, and I'm guessing you
treated the two fields as a single operand type because OT_REG_ELEMENT
is a single token.  Is that right?  I think I'd prefer to split the
tokens instead.  E.g. something like the attached.  OT_REG_INDEX is unused
until your patch.

With that change, I'm hoping the registers could just be normal OP_REGs
and the indices could use just two operand types, one for constant indices
and one for register indices.  The range of the constant indices would then
be obvious from their size, so we wouldn't need different OP_s for
0..3, 0..7, etc.  The "always 0" case would be a 0-sized operand.

BTW:

> -	  my_getExpression (&element, s);
> -	  if (element.X_op != O_constant)
> +	  if (*s == '$')
>  	    {
> -	      set_insn_error (0, _("vector element must be constant"));
> -	      return 0;
> +	      token.u.reg_element.is_reg_index_p = TRUE;
> +	      if (!mips_parse_register (&s, &regno3, NULL))
> +		{
> +		  set_insn_error (0, _("invalid register"));
> +		  return 0;
> +		}
> +	      temp_index = regno3;
> +	    }

$ can be used for general symbols too, in something like:

	.equ	$my_index, 1
	...[$my_index]

which should be the same as "...[1]".  The patch below tries parsing the
register first and falls back to the expression on failure.

Thanks,
Richard


gas/
	* config/tc-mips.c (OT_REG_ELEMENT): Replace with...
	(OT_INTEGER_INDEX, OT_REG_INDEX): ...these new operand types.
	(mips_operand_token): Replace reg_element with index.
	(mips_parse_argument_token): Treat vector indices as separate tokens.
	Handle register indices.
	(match_mdmx_imm_reg_operand): Update accordingly.

Index: gas/config/tc-mips.c
===================================================================
--- gas/config/tc-mips.c	2013-09-24 21:49:25.688414012 +0100
+++ gas/config/tc-mips.c	2013-10-08 19:56:07.848684599 +0100
@@ -2747,8 +2747,11 @@ enum mips_operand_token_type {
   /* A 4-bit XYZW channel mask.  */
   OT_CHANNELS,
 
-  /* An element of a vector, e.g. $v0[1].  */
-  OT_REG_ELEMENT,
+  /* A constant vector index, e.g. [1].  */
+  OT_INTEGER_INDEX,
+
+  /* A register vector index, e.g. [$2].  */
+  OT_REG_INDEX,
 
   /* A continuous range of registers, e.g. $s0-$s4.  */
   OT_REG_RANGE,
@@ -2777,17 +2780,14 @@ struct mips_operand_token
   enum mips_operand_token_type type;
   union
   {
-    /* The register symbol value for an OT_REG.  */
+    /* The register symbol value for an OT_REG or OT_REG_INDEX.  */
     unsigned int regno;
 
     /* The 4-bit channel mask for an OT_CHANNEL_SUFFIX.  */
     unsigned int channels;
 
-    /* The register symbol value and index for an OT_REG_ELEMENT.  */
-    struct {
-      unsigned int regno;
-      addressT index;
-    } reg_element;
+    /* The integer value of an OT_INTEGER_INDEX.  */
+    addressT index;
 
     /* The two register symbol values involved in an OT_REG_RANGE.  */
     struct {
@@ -2948,20 +2948,32 @@ mips_parse_argument_token (char *s, char
 	  mips_add_token (&token, OT_REG_RANGE);
 	  return s;
 	}
-      else if (*s == '[')
-	{
-	  /* A vector element.  */
-	  expressionS element;
 
+      /* Add the register itself.  */
+      token.u.regno = regno1;
+      mips_add_token (&token, OT_REG);
+
+      /* Check for a vector index.  */
+      if (*s == '[')
+	{
 	  ++s;
 	  SKIP_SPACE_TABS (s);
-	  my_getExpression (&element, s);
-	  if (element.X_op != O_constant)
+	  if (mips_parse_register (&s, &token.u.regno, NULL))
+	    mips_add_token (&token, OT_REG_INDEX);
+	  else
 	    {
-	      set_insn_error (0, _("vector element must be constant"));
-	      return 0;
+	      expressionS element;
+
+	      my_getExpression (&element, s);
+	      if (element.X_op != O_constant)
+		{
+		  set_insn_error (0, _("vector element must be constant"));
+		  return 0;
+		}
+	      s = expr_end;
+	      token.u.index = element.X_add_number;
+	      mips_add_token (&token, OT_INTEGER_INDEX);
 	    }
-	  s = expr_end;
 	  SKIP_SPACE_TABS (s);
 	  if (*s != ']')
 	    {
@@ -2969,16 +2981,7 @@ mips_parse_argument_token (char *s, char
 	      return 0;
 	    }
 	  ++s;
-
-	  token.u.reg_element.regno = regno1;
-	  token.u.reg_element.index = element.X_add_number;
-	  mips_add_token (&token, OT_REG_ELEMENT);
-	  return s;
 	}
-
-      /* Looks like just a plain register.  */
-      token.u.regno = regno1;
-      mips_add_token (&token, OT_REG);
       return s;
     }
 
@@ -5078,7 +5081,7 @@ match_mdmx_imm_reg_operand (struct mips_
   uval = mips_extract_operand (operand, opcode->match);
   is_qh = (uval != 0);
 
-  if (arg->token->type == OT_REG || arg->token->type == OT_REG_ELEMENT)
+  if (arg->token->type == OT_REG)
     {
       if ((opcode->membership & INSN_5400)
 	  && strcmp (opcode->name, "rzu.ob") == 0)
@@ -5088,20 +5091,21 @@ match_mdmx_imm_reg_operand (struct mips_
 	  return FALSE;
 	}
 
+      if (!match_regno (arg, OP_REG_VEC, arg->token->u.regno, &regno))
+	return FALSE;
+      ++arg->token;
+
       /* Check whether this is a vector register or a broadcast of
 	 a single element.  */
-      if (arg->token->type == OT_REG_ELEMENT)
+      if (arg->token->type == OT_INTEGER_INDEX)
 	{
-	  if (!match_regno (arg, OP_REG_VEC, arg->token->u.reg_element.regno,
-			    &regno))
-	    return FALSE;
-	  if (arg->token->u.reg_element.index > (is_qh ? 3 : 7))
+	  if (arg->token->u.index > (is_qh ? 3 : 7))
 	    {
 	      set_insn_error (arg->argnum, _("invalid element selector"));
 	      return FALSE;
 	    }
-	  else
-	    uval |= arg->token->u.reg_element.index << (is_qh ? 2 : 1) << 5;
+	  uval |= arg->token->u.index << (is_qh ? 2 : 1) << 5;
+	  ++arg->token;
 	}
       else
 	{
@@ -5115,15 +5119,12 @@ match_mdmx_imm_reg_operand (struct mips_
 	      return FALSE;
 	    }
 
-	  if (!match_regno (arg, OP_REG_VEC, arg->token->u.regno, &regno))
-	    return FALSE;
 	  if (is_qh)
 	    uval |= MDMX_FMTSEL_VEC_QH << 5;
 	  else
 	    uval |= MDMX_FMTSEL_VEC_OB << 5;
 	}
       uval |= regno;
-      ++arg->token;
     }
   else
     {