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] Add support for MIPS64r6


Sorry for taking so long to get to this.

Andrew Bennett <Andrew.Bennett@imgtec.com> writes:
> This patch adds support for MIPS64r6.  MIPS64r6 includes most of MIPS64r5 
> but there are some instructions that have been re-coded or removed in order 
> to support the new MIPS64r6 instructions.  
>
> For more information please refer to:
> http://www.imgtec.com/mips/mips64-architecture.asp
>
> Some notes on the changes:
>
> A dense encoding has been used for several new instructions leading to the
> introduction of new special operand types. Some special casing during
> disassembly has also been necessary to handle these cases. While some checks
> are redundant due to the ordering of the opcode table they are present for
> safety in case of any future re-ordering of the table.
>
> A new PLT template has been added for R6 as the explicit JR opcode has been
> removed instead relying on JALR $0, <reg>.  Although JALR exists for <=r5
> we are being cautious in case the use of JALR is interpreted as a call in
> pre-existing MIPS implementations as part of hardware optimisations.
>
> MicroMIPSr5 is not compatible with MIPSr6 and as such is disabled. The JALX
> instruction has also been removed and the encoding re-used. If a new
> compressed ISA is defined then the interlinking support will need updating.
>
> ABI related changes for R6 are:
> 1) Introduction of new PC relative relocations
> 2) Definition of the new ISAs in e_flags.
>
> We would like to get early feedback on the ABI so that work in other tools
> can also be finalised.  The decision to define the new ISA in e_flags instead
> of the newly proposed .MIPS.abiflags section is due to the need for preventing
> linking of R<6 and R6 code.  While mixing ISAs within executables and DSOs is
> not supported we do not intend to prevent mixing of executables and DSOs.  The
> linux kernel will handle the emulation of instructions that do not exist.

This all looks good to me FWIW.

> The current patch does not contain any test cases, as we would like your 
> opinions on how to proceed with this.  There are three main issues.  Firstly,
> disabling all tests that check instructions that have been removed in 
> MIPSr6.  The problem here is that typically these tests contain valid, and
> non-valid R6 instructions, so there needs to be a way to prevent the non-valid
> instructions from being assembled if we are testing for R6.  One approach is to
> break the testcases up into the valid and non-valid instructions, and then just
> predicate the non-valid instruction tests in mips.exp for R6. What do you think
> about doing this?

Typically we run those tests via run_dump_test_arch, so we could just
add a "--defsym r6=0" or "--defsym r6=1" to the assembler command line
and use .if-based directives to skip the removed instructions.

> Secondly, updating tests that check for instructions that have been recoded in
> R6.  Would you be happy if we used the micromips@ approach for the disassembly
> files; we could use something like mipsr6@ ?

Yeah, that sounds like the right way to do it.  Or more specifically,
if the architecture extends mips32r6 and mips64r6, check for mipsr6@.

> Finally, should we cover all new instructions in one file and all removed
> instructions in another or break it down further?

I don't really have a strong opinion about that.  If you prefer one of the
other, go for it, otherwise maybe single tests make it easier to check
that all insns are covered?

> diff --git a/bfd/archures.c b/bfd/archures.c
> index 4ab5f1d..92bf821 100644
> --- a/bfd/archures.c
> +++ b/bfd/archures.c
> @@ -182,8 +182,10 @@ DESCRIPTION
>  .#define bfd_mach_mips_xlr              887682   {* decimal 'XLR'  *}
>  .#define bfd_mach_mipsisa32             32
>  .#define bfd_mach_mipsisa32r2           33
> +.#define bfd_mach_mipsisa32r6           34
>  .#define bfd_mach_mipsisa64             64
>  .#define bfd_mach_mipsisa64r2           65
> +.#define bfd_mach_mipsisa64r6           66
>  .#define bfd_mach_mips_micromips        96
>  .  bfd_arch_i386,      {* Intel 386 *}
>  .#define bfd_mach_i386_intel_syntax	(1 << 0)

Obviously r3 and r5 have gone in since you posted this so now it
should be 37 and 69.

> +static const bfd_vma mipsr6_exec_plt_entry[] =
> +{
> +  0x3c0f0000,	/* lui $15, %hi(.got.plt entry)			*/
> +  0x01f90000,	/* l[wd] $25, %lo(.got.plt entry)($15)		*/
> +  0x25f80000,	/* addiu $24, $15, %lo(.got.plt entry)		*/
> +  0x03200009	/* jr $25					*/
> +};

We don't need to worry about MIPS I compatibility here, so we could put
the JR before the ADDIU and avoid executing the LUI from the following
PLT.  What you have is fine too if you think it's better though.

> +    case R_MIPS_PCHI16:
> +      if (howto->partial_inplace)
> +	addend = _bfd_mips_elf_sign_extend (addend, 16);
> +      value = mips_elf_high (symbol + addend - p);
> +      BFD_ASSERT (howto->rightshift == 16);
> +      overflowed_p = mips_elf_overflow_p (value, 16);
> +      value &= howto->dst_mask;
> +      break;

Is the REL handling deliberately different from other HI16s here?
Normally the idea is that the HI16 is paired with a following LO16
and you combine the in-place addends from both to get the full
HI16 addend.  "addend" would then already be the full addend
in this case.

> +    case R_MIPS_PCLO16:
> +      if (howto->partial_inplace)
> +	addend = _bfd_mips_elf_sign_extend (addend, 16);
> +      value = symbol + addend - p;
> +      overflowed_p = mips_elf_overflow_p (value, 32);
> +      value &= howto->dst_mask;
> +      break;

LO16s can't overflow.  I think this should just be:

      value = symbol + _bfd_mips_elf_sign_extend (addend, 16) - p;
      value &= howto->dst_mask;
      break;

with the overflow check being left to the HI16.

> @@ -10048,6 +10141,13 @@ _bfd_mips_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
>  		(info, msg, name, input_bfd, input_section, rel->r_offset);
>  	      return FALSE;
>  	    }
> +	  if (aligned_pcrel_reloc_p (howto->type))
> +	    {
> +	      msg = _("pc relative load using unaligned address");

"PC-relative".  Maybe also "from" rather than "using"?

> diff --git a/binutils/testsuite/binutils-all/objcopy.exp b/binutils/testsuite/binutils-all/objcopy.exp
> index 6159b9d..9b22744 100644
> --- a/binutils/testsuite/binutils-all/objcopy.exp
> +++ b/binutils/testsuite/binutils-all/objcopy.exp
> @@ -986,6 +986,7 @@ if [is_elf_format] {
>      # targ_defvec=bfd_elf32_nlittlemips_vec in config.bfd.  When syncing,
>      # don't forget that earlier case-matches trump later ones.
>      if { ![istarget "mips*-sde-elf*"] && ![istarget "mips*-mti-elf*"]
> +	 && ![istarget "mips*-img-elf*"]
>           && ![istarget "mips64*-*-openbsd*"] } {
>  	setup_xfail "mips*-*-irix5*" "mips*-*-irix6*" "mips*-*-elf*" \
>  	    "mips*-*-rtems*" "mips*-*-windiss" "mips*-*-none" \
> diff --git a/binutils/testsuite/binutils-all/readelf.exp b/binutils/testsuite/binutils-all/readelf.exp
> index 2a6bc6a..e45d6ea 100644
> --- a/binutils/testsuite/binutils-all/readelf.exp
> +++ b/binutils/testsuite/binutils-all/readelf.exp
> @@ -103,6 +103,7 @@ proc readelf_test { options binary_file regexp_file xfails } {
>  	if { [istarget "mips*-*-*linux*"]
>  	     || [istarget "mips*-sde-elf*"]
>  	     || [istarget "mips*-mti-elf*"]
> +	     || [istarget "mips*-img-elf*"]
>  	     || [istarget "mips*-*freebsd*"] } then {
>  	    set target_machine tmips
>  	} else {

Please submit the img-elf bits separately.

> +/* OP_SAME_RS_RT matcher.  */
> +
> +static bfd_boolean
> +match_same_rs_rt_operand (struct mips_arg_info *arg,
> +			  const struct mips_operand *operand)
> +{
> +  unsigned int regno;
> +
> +  if (!match_reg (arg, OP_REG_GP, &regno))
> +    return FALSE;
> +
> +  if (regno == 0)
> +    {
> +      set_insn_error (arg->argnum, _("the source register must not be $0"));
> +      return FALSE;
> +    }
> +
> +  arg->last_regno = regno;
> +
> +  insn_insert_operand (arg->insn, operand, regno | (regno << 5));
> +  return TRUE;
> +}

Looks good, but after this...

> +
> +/* OP_GP_NOT_ZERO matcher.  */
> +
> +static bfd_boolean
> +match_gp_not_zero_operand (struct mips_arg_info *arg,
> +			   const struct mips_operand *operand)
> +{
> +  unsigned int regno;
> +
> +  if (!match_reg (arg, OP_REG_GP, &regno))
> +    return FALSE;
> +
> +  if (regno == 0)
> +    {
> +      set_insn_error (arg->argnum, _("the source register must not be $0"));
> +      return FALSE;
> +    }
> +
> +  arg->last_regno = regno;
> +
> +  insn_insert_operand (arg->insn, operand, regno);
> +  return TRUE;
> +}
> +
> +/* OP_GP_NOT_ZERO_LT_PREV matcher.  */
> +
> +static bfd_boolean
> +match_gp_not_zero_lt_prev_operand (struct mips_arg_info *arg,
> +				   const struct mips_operand *operand)
> +{
> +  unsigned int regno;
> +
> +  if (!match_reg (arg, OP_REG_GP, &regno))
> +    return FALSE;
> +
> +  if (regno == 0)
> +    {
> +      set_insn_error (arg->argnum, _("the source register must not be $0"));
> +      return FALSE;
> +    }
> +
> +  if (regno >= arg->last_regno)
> +    return FALSE;
> +
> +  arg->last_regno = regno;
> +
> +  insn_insert_operand (arg->insn, operand, regno);
> +  return TRUE;
> +}
> +
> +/* OP_GP_GT_PREV matcher.  */
> +
> +static bfd_boolean
> +match_gp_gt_prev_operand (struct mips_arg_info *arg,
> +			  const struct mips_operand *operand)
> +{
> +  unsigned int regno;
> +
> +  if (!match_reg (arg, OP_REG_GP, &regno))
> +    return FALSE;
> +
> +  if (regno <= arg->last_regno)
> +    return FALSE;
> +
> +  arg->last_regno = regno;
> +
> +  insn_insert_operand (arg->insn, operand, regno);
> +  return TRUE;
> +}
> +
> +/* OP_GP_LE_PREV matcher.  */
> +
> +static bfd_boolean
> +match_gp_le_prev_operand (struct mips_arg_info *arg,
> +			  const struct mips_operand *operand)
> +{
> +  unsigned int regno;
> +
> +  if (!match_reg (arg, OP_REG_GP, &regno))
> +    return FALSE;
> +
> +  if (regno > arg->last_regno)
> +    return FALSE;
> +
> +  arg->last_regno = regno;
> +
> +  insn_insert_operand (arg->insn, operand, regno);
> +  return TRUE;
> +}
> +
> +/* OP_GP_GE_PREV matcher.  */
> +
> +static bfd_boolean
> +match_gp_ge_prev_operand (struct mips_arg_info *arg,
> +			  const struct mips_operand *operand)
> +{
> +  unsigned int regno;
> +
> +  if (!match_reg (arg, OP_REG_GP, &regno))
> +    return FALSE;
> +
> +  if (regno < arg->last_regno)
> +    return FALSE;
> +
> +  arg->last_regno = regno;
> +
> +  insn_insert_operand (arg->insn, operand, regno);
> +  return TRUE;
> +}
> +
> +/* OP_GP_NOT_ZERO_NOT_PREV matcher.  */
> +
> +static bfd_boolean
> +match_gp_not_zero_not_prev_operand (struct mips_arg_info *arg,
> +				    const struct mips_operand *operand)
> +{
> +  unsigned int regno;
> +
> +  if (!match_reg (arg, OP_REG_GP, &regno))
> +    return FALSE;
> +
> +  if (regno == 0 || regno == arg->last_regno)
> +    {
> +      set_insn_error (arg->argnum, _("the source registers must not be $0 and different"));
> +      return FALSE;
> +    }
> +
> +  arg->last_regno = regno;
> +
> +  insn_insert_operand (arg->insn, operand, regno);
> +  return TRUE;
> +}

...there's a lot of commonality.  Please try to find some way of factoring
this.  One way would be to have a new mips_operand-derived structure
(along the lines of mips_reg_pair_operand) that says which of <, =, and >
are acceptable and whether $0 is allowed.  OP_REPEAT_PREV_REG could
use it too.

> @@ -5245,11 +5483,13 @@ match_pc_operand (struct mips_arg_info *arg)
>     register that we need to match.  */
>  
>  static bfd_boolean
> -match_tied_reg_operand (struct mips_arg_info *arg, unsigned int other_regno)
> +match_tied_reg_operand (struct mips_arg_info *arg,
> +			enum mips_reg_operand_type type,
> +			unsigned int other_regno)
>  {
>    unsigned int regno;
>  
> -  return match_reg (arg, OP_REG_GP, &regno) && regno == other_regno;
> +  return match_reg (arg, type, &regno) && regno == other_regno;
>  }
>  
>  /* Read a floating-point constant from S for LI.S or LI.D.  LENGTH is
> @@ -5495,10 +5735,10 @@ match_operand (struct mips_arg_info *arg,
>        return match_mdmx_imm_reg_operand (arg, operand);
>  
>      case OP_REPEAT_DEST_REG:
> -      return match_tied_reg_operand (arg, arg->dest_regno);
> +      return match_tied_reg_operand (arg, OP_REG_GP, arg->dest_regno);
>  
>      case OP_REPEAT_PREV_REG:
> -      return match_tied_reg_operand (arg, arg->last_regno);
> +      return match_tied_reg_operand (arg, OP_REG_GP, arg->last_regno);
>  
>      case OP_PC:
>        return match_pc_operand (arg);

Couldn't tell off-hand why you need this (although it looks OK).

> @@ -5714,6 +5976,11 @@ insns_between (const struct mips_cl_insn *insn1,
>  	return 1;
>      }
>  
> +  if (insn1->insn_mo->pinfo2 & INSN2_FORBIDDEN_SLOT
> +      && (pinfo2 & INSN_NO_DELAY_SLOT
> +	  || (insn2 && delayed_branch_p (insn2))))
> +    return 1;
> +

Please add a comment and use (....) round the "&"s.

> @@ -6571,6 +6838,46 @@ append_insn (struct mips_cl_insn *ip, expressionS *address_expr,
>  	  }
>  	  break;
>  
> +	case BFD_RELOC_MIPS_21_PCREL_S2:
> +	  {
> +	    int shift;
> +
> +	    shift = 2;
> +	    if ((address_expr->X_add_number & ((1 << shift) - 1)) != 0)
> +	      as_bad (_("branch to misaligned address (0x%lx)"),
> +		      (unsigned long) address_expr->X_add_number);
> +	    if (!mips_relax_branch)
> +	      {
> +		if ((address_expr->X_add_number + (1 << (shift + 20)))
> +		    & ~((1 << (shift + 21)) - 1))
> +		  as_bad (_("branch address range overflow (0x%lx)"),
> +			  (unsigned long) address_expr->X_add_number);
> +		ip->insn_opcode |= ((address_expr->X_add_number >> shift)
> +				    & 0x1fffff);
> +	      }
> +	  }
> +	  break;
> +
> +	case BFD_RELOC_MIPS_26_PCREL_S2:
> +	  {
> +	    int shift;
> +
> +	    shift = 2;
> +	    if ((address_expr->X_add_number & ((1 << shift) - 1)) != 0)
> +	      as_bad (_("branch to misaligned address (0x%lx)"),
> +		      (unsigned long) address_expr->X_add_number);
> +	    if (!mips_relax_branch)
> +	      {
> +		if ((address_expr->X_add_number + (1 << (shift + 25)))
> +		    & ~((1 << (shift + 26)) - 1))
> +		  as_bad (_("branch address range overflow (0x%lx)"),
> +			  (unsigned long) address_expr->X_add_number);
> +		ip->insn_opcode |= ((address_expr->X_add_number >> shift)
> +				    & 0x3ffffff);
> +	      }
> +	  }
> +	  break;

I think we should drop the !mips_relax_branch test until branch relaxation
is supported for r6.

> @@ -14226,6 +14583,17 @@ mips_force_relocation (fixS *fixp)
>        || fixp->fx_r_type == BFD_RELOC_MICROMIPS_16_PCREL_S1)
>      return 1;
>  
> +  /* We want all relocations to be kept for R6 relaxation */
> +  if (ISA_IS_R6 (mips_opts.isa)
> +      && (fixp->fx_r_type == BFD_RELOC_16_PCREL_S2
> +	  || fixp->fx_r_type == BFD_RELOC_MIPS_21_PCREL_S2
> +	  || fixp->fx_r_type == BFD_RELOC_MIPS_26_PCREL_S2
> +	  || fixp->fx_r_type == BFD_RELOC_MIPS_18_PCREL_S3
> +	  || fixp->fx_r_type == BFD_RELOC_MIPS_19_PCREL_S2
> +	  || fixp->fx_r_type == BFD_RELOC_HI16_S_PCREL
> +	  || fixp->fx_r_type == BFD_RELOC_LO16_PCREL))
> +    return 1;

"all PC-relative relocations"?

> @@ -14462,6 +14836,81 @@ md_apply_fix (fixS *fixP, valueT *valP, segT seg ATTRIBUTE_UNUSED)
>  	md_number_to_chars (buf, *valP, fixP->fx_size);
>        break;
>  
> +    case BFD_RELOC_MIPS_21_PCREL_S2:
> +      if ((*valP & 0x3) != 0)
> +	as_bad_where (fixP->fx_file, fixP->fx_line,
> +		      _("branch to misaligned address (%lx)"), (long) *valP);
> +
> +      /* We need to save the bits in the instruction since fixup_segment()
> +	 might be deleting the relocation entry (i.e., a branch within
> +	 the current segment).  */
> +      if (! fixP->fx_done)
> +	break;
> +
> +      /* Update old instruction data.  */
> +      insn = read_insn (buf);
> +
> +      if (*valP + 0x400000 <= 0x7fffff)
> +	{
> +	  insn |= (*valP >> 2) & 0x1fffff;
> +	  write_insn (buf, insn);
> +	}
> +      else
> +	{
> +	  /* CFU FIXME.  */
> +	  gas_assert (0);
> +	}
> +      break;
> +
> +    case BFD_RELOC_MIPS_26_PCREL_S2:
> +      if ((*valP & 0x3) != 0)
> +	as_bad_where (fixP->fx_file, fixP->fx_line,
> +		      _("branch to misaligned address (%lx)"), (long) *valP);
> +
> +      /* We need to save the bits in the instruction since fixup_segment()
> +	 might be deleting the relocation entry (i.e., a branch within
> +	 the current segment).  */
> +      if (! fixP->fx_done)
> +	break;
> +
> +      /* Update old instruction data.  */
> +      insn = read_insn (buf);
> +
> +      if (*valP + 0x8000000 <= 0xfffffff)
> +	{
> +	  insn |= (*valP >> 2) & 0x3ffffff;
> +	  write_insn (buf, insn);
> +	}
> +      else
> +	{
> +	  /* CFU FIXME.  */
> +	  gas_assert (0);
> +	}
> +      break;

Can fx_done ever happen given the mips_force_relocation change?
I think we should assert if not, like you do for the others.

> +    case BFD_RELOC_MIPS_18_PCREL_S3:
> +      if ((*valP & 0x3) != 0)
> +	as_bad_where (fixP->fx_file, fixP->fx_line,
> +		      _("pc rel from misaligned address (%lx)"),
> +		      (long) *valP);
> +
> +      gas_assert(!fixP->fx_done);
> +      break;

Should this be 7 rather than 3?

> diff --git a/gas/doc/c-mips.texi b/gas/doc/c-mips.texi
> index 0c5e82d..ffcf27f 100644
> --- a/gas/doc/c-mips.texi
> +++ b/gas/doc/c-mips.texi
> @@ -81,17 +81,20 @@ VxWorks-style position-independent macro expansions.
>  @itemx -mips5
>  @itemx -mips32
>  @itemx -mips32r2
> +@itemx -mips32r6
>  @itemx -mips64
>  @itemx -mips64r2
> +@itemx -mips64r6
>  Generate code for a particular MIPS Instruction Set Architecture level.
>  @samp{-mips1} corresponds to the R2000 and R3000 processors,
>  @samp{-mips2} to the R6000 processor, @samp{-mips3} to the
>  R4000 processor, and @samp{-mips4} to the R8000 and R10000 processors.
> -@samp{-mips5}, @samp{-mips32}, @samp{-mips32r2}, @samp{-mips64}, and
> -@samp{-mips64r2} correspond to generic MIPS V, MIPS32, MIPS32 Release 2,
> -MIPS64, and MIPS64 Release 2 ISA processors, respectively.  You can also
> -switch instruction sets during the assembly; see @ref{MIPS ISA,
> -Directives to override the ISA level}.
> +@samp{-mips5}, @samp{-mips32}, @samp{-mips32r2}, @samp{-mips32r6},
> +@samp{-mips64}, @samp{-mips64r2} and @samp{-mips64r6} correspond to
> +generic MIPS V, MIPS32, MIPS32 Release 2, MIPS32 Release 6, MIPS64,
> +MIPS64 Release 2, and MIPS64 Release 6 ISA processors, respectively.
> +You can also switch instruction sets during the assembly; see
> +@ref{MIPS ISA, Directives to override the ISA level}.
>  
>  @item -mgp32
>  @itemx -mfp32
> @@ -652,8 +655,8 @@ Small data is not supported for SVR4-style PIC.
>  @kindex @code{.set mips@var{n}}
>  @sc{gnu} @code{@value{AS}} supports an additional directive to change
>  the MIPS Instruction Set Architecture level on the fly: @code{.set
> -mips@var{n}}.  @var{n} should be a number from 0 to 5, or 32, 32r2, 64
> -or 64r2.
> +mips@var{n}}.  @var{n} should be a number from 0 to 5, or 32, 32r2,
> +32r6, 64, 64r2 or 64r6.
>  The values other than 0 make the assembler accept instructions
>  for the corresponding ISA level, from that point on in the
>  assembly.  @code{.set mips@var{n}} affects not only which instructions

This needs to be duplicated in as.texinfo.

> @@ -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?

> @@ -1077,6 +1096,23 @@ print_vu0_channel (struct disassemble_info *info,
>      abort ();
>  }
>  
> +/* Record information about a register operand */
> +
> +static void
> +mips_seen_register (struct mips_print_arg_state *state,
> +		    unsigned int regno,
> +		    enum mips_reg_operand_type reg_type)
> +{
> +  state->last_reg_type = reg_type;
> +  state->last_regno = regno;
> +
> +  if (!state->seen_dest)
> +    {
> +      state->seen_dest = 1;
> +      state->dest_regno = regno;
> +    }
> +}

Nice.  Thanks for factoring this out.

> @@ -1358,13 +1473,14 @@ print_insn_arg (struct disassemble_info *info,
>  
>  /* Print the arguments for INSN, which is described by OPCODE.
>     Use DECODE_OPERAND to get the encoding of each operand.  Use BASE_PC
> -   as the base of OP_PCREL operands.  */
> +   as the base of OP_PCREL operands adjusting by LENGTH if the OP_PCREL
> +   operand is for a branch or jump.  */

Comma before "adjusting".

> @@ -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.

>  	      /* We always allow to disassemble the jalx instruction.  */
>  	      if (!opcode_is_member (op, mips_isa, mips_ase, mips_processor)
> -		  && strcmp (op->name, "jalx"))
> +		 && (strcmp (op->name, "jalx")
> +		     || (mips_isa & INSN_ISA_MASK) == ISA_MIPS32R6
> +		     || (mips_isa & INSN_ISA_MASK) == ISA_MIPS64R6))
>  		continue;

Comment needs updating.

I'm going to take the opcodes changes on faith :-)

Thanks,
Richard


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