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

[Richard Sandiford <rdsandiford at googlemail dot com>]

Chao-Ying Fu <Chao-Ying.Fu@imgtec.com> writes:
> 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.

The flow is that mips_parse_arguments is first called to parse all
the arguments in one go.  It converts them to an array of
mips_operand_tokens.  This array is then used instead of string
parsing when matching the operands.  (The parsing only happens
once per .s instruction regardless of how many opcode alternatives
we have to match against.  Nothing other than mips_parse_arguments
calls mips_parse_argument_token.)

At the moment "xx[yy]" is one token, but my patch split it into two,
a normal OT_REG for the "xx" followed by an OT_INTEGER_INDEX or
OT_REG_INDEX for the "[yy]".  So the array will contain two tokens
that can be individually matched as two operands.

"xx[yy]" could then be represented as "+a+b" in the opcode/*.c table
(pretending that "a" and "b" are free).  "+a" would be a normal OP_REG.
"+b" would be a new OP_*.  And the match_operand routine for that new
OP_* would just check whether the next token in the array is a
OT_INTEGER_INDEX or OT_REG_INDEX.

Thanks,
Richard