Re: [PATCH] Add support for MIPS64r6

[Richard Sandiford <rdsandiford at googlemail dot com>]

Andrew Bennett <Andrew.Bennett@imgtec.com> writes:
>> > @@ -1089,7 +1146,8 @@ struct mips_opcode
>> >     (mips_isa_table[(Y & INSN_ISA_MASK) - 1] >> ((X & INSN_ISA_MASK) - 1)) &
>> 1
>> >     is non-zero.  */
>> >  static const unsigned int mips_isa_table[] =
>> > -  { 0x0001, 0x0003, 0x0607, 0x1e0f, 0x3e1f, 0x0a23, 0x3e63, 0x3ebf, 0x3fff
>> };
>> > +  { 0x0001, 0x0003, 0x0607, 0x1e0f, 0x3e1f, 0x0a23, 0x3e63, 0x3ebf, 0x3fff,
>> > +    0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x7e63, 0xffff };
>> 
>> Is it really true that r6 allows everything, given the incompatibility?
>
> The problem is that the removed instructions in R6 come from different
> ISAs.  One approach to solve this is to describe in the membership
> field the different ISAs an instruction belongs to.  This would
> require us to create a large amount of different ISA combinations
> which is hard to manage.  A cleaner approach (and implemented in the
> patch) is to say that R6 is an extension of R5 and then to deal with
> the removed instructions by adding instruction exclusions for R6.

OK, sounds good, thanks.  Please add a comment along those lines.

>> > @@ -1426,9 +1542,27 @@ print_insn_args (struct disassemble_info *info,
>> >  		infprintf (is, "$%d,%d", reg, sel);
>> >  	    }
>> >  	  else
>> > -	    print_insn_arg (info, &state, opcode, operand, base_pc,
>> > -			    mips_extract_operand (operand, insn));
>> > -	  if (*s == 'm' || *s == '+')
>> > +	    {
>> > +	      bfd_vma base_pc = insn_pc;
>> > +
>> > +	      /* Adjust the PC relative base so that branch/jump insns use
>> > +		 the following PC as the base but genuinely PC relative
>> > +		 operands use the current PC.  */
>> > +	      if (operand->type == OP_PCREL)
>> > +		{
>> > +		  const struct mips_pcrel_operand *pcrel_op;
>> > +
>> > +		  pcrel_op = (const struct mips_pcrel_operand *) operand;
>> > +		  /* The include_isa_bit flag is sufficient to distinguish
>> > +		     branch/jump from other PC relative operands.  */
>> > +		  if (pcrel_op->include_isa_bit)
>> > +		    base_pc += length;
>> > +		}
>> > +
>> > +	      print_insn_arg (info, &state, opcode, operand, base_pc,
>> > +			      mips_extract_operand (operand, insn));
>> > +	    }
>> > +	  if (*s == 'm' || *s == '+' || *s == '-')
>> >  	    ++s;
>> >  	  break;
>> >  	}
>> > @@ -1494,9 +1628,60 @@ print_insn_mips (bfd_vma memaddr,
>> >  	      && !(no_aliases && (op->pinfo2 & INSN2_ALIAS))
>> >  	      && (word & op->mask) == op->match)
>> >  	    {
>> > +	      if (strcmp (op->name, "bgezc") == 0
>> > +		  || strcmp (op->name, "bltzc") == 0
>> > +		  || strcmp (op->name, "bgezalc") == 0
>> > +		  || strcmp (op->name, "bltzalc") == 0)
>> > +		{
>> > +		  if (((word >> 16) & 31) != ((word >> 21) & 31)
>> > +		      || ((word >> 16) & 31) == 0)
>> > +		    continue;
>> > +		}
>> > +	      else if (strcmp (op->name, "blezalc") == 0
>> > +		       || strcmp (op->name, "bgtzalc") == 0
>> > +		       || strcmp (op->name, "blezc") == 0
>> > +		       || strcmp (op->name, "bgtzc") == 0
>> > +		       || strcmp (op->name, "beqzalc") == 0
>> > +		       || strcmp (op->name, "bnezalc") == 0)
>> > +		{
>> > +		  if (((word >> 16) & 31) == 0)
>> > +		    continue;
>> > +		}
>> > +	      else if (strcmp (op->name, "bgec") == 0
>> > +		       || strcmp (op->name, "bltc") == 0
>> > +		       || strcmp (op->name, "bbec") == 0
>> > +		       || strcmp (op->name, "bstc") == 0)
>> > +		{
>> > +		  if (((word >> 16) & 31) == ((word >> 21) & 31)
>> > +		      || ((word >> 21) & 31) == 0
>> > +		      || ((word >> 16) & 31) == 0)
>> > +		    continue;
>> > +		}
>> > +	      else if (strcmp (op->name, "beqc") == 0
>> > +		       || strcmp (op->name, "bnec") == 0)
>> > +		{
>> > +		  if (((word >> 21) & 31) >= ((word >> 16) & 31)
>> > +		      || ((word >> 21) & 31) == 0)
>> > +		    continue;
>> > +		}
>> > +	      else if (strcmp (op->name, "bovc") == 0
>> > +		       || strcmp (op->name, "bnvc") == 0)
>> > +		{
>> > +		  if (((word >> 21) & 31) < ((word >> 16) & 31))
>> > +		    continue;
>> > +		}
>> > +	      else if (strcmp (op->name, "beqzc") == 0
>> > +		       || strcmp (op->name, "bnezc") == 0)
>> > +		{
>> > +		  if (((word >> 21) & 31) == 0)
>> > +		    continue;
>> > +		}
>> 
>> Looks like this is just reinforcing the restrictions from the OP_*s,
>> is that right?  If so, I think it would be cleaner to use the
>> mips_operand information rather than checks for specific instructions.
>
> I agree with you that the code could be better here.  The problem is
> that for some of the R6 instructions the match and mask fields are
> identical (one example is the bnvc and bnec instructions).  Currently
> the print_insn_mips function uses the match and mask fields to find
> the first instruction in the opcode table which matches the
> instruction to disassemble.  Then it takes each of the operands in
> turn and checks that they are valid.  This approach will obviously not
> work when decoding some R6 instructions.  The current fix is to skip
> over R6 instructions where the operands don't match (which as we said
> before is not very maintainable).  We could do a more generic fix, but
> that would require rewriting the print_insn_mips, print_insn_args, and
> print_insn_arg functions to find an instruction in the opcode table
> where firstly the match and mask fields match the instruction to
> disassemble; and secondly all the operand constraints are met.  If
> this is the case the instruction could then be printed out.
>
> I was wondering what you felt would be the best approach here?

Yeah, rewriting it to treat the new mips_operand type as part of the
matching criteria sounds better to me.  I realise that might be quite
invasive, but to be fair, it wasn't easy to predict that this would
happen when the code was written :-)  I think we should extend what's
there rather than work around it.

Thanks,
Richard