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

[Chao-Ying Fu <Chao-Ying dot Fu at imgtec dot com>]

Richard Sandiford wrote:
> 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.

  Thanks a lot for your patch!  However, the issue is that after matching
a first register, mips_parse_argument_token() continues to match an '[' and an index and ']'.
This implies that if an instruction has a format of "xx[yy]", xx and yy will be
parsed together as one operand.
I am not sure splitting to two tokens can help to parse xx and yy via
calling mips_parse_argument_token() twice.
So, for MSA instructions, I combine "xx[yy]" into one operand.
Or, I may misunderstand the parsing code.

> 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 for pointing out this example!  Yes, my patch fails on this example.

Regards,
Chao-ying