Re: [PATCH] Add support for MIPS64r6

[Richard Sandiford <rdsandiford at googlemail dot com>]

Andrew Bennett <Andrew.Bennett@imgtec.com> writes:
> diff --git a/bfd/config.bfd b/bfd/config.bfd
> index 1f822f2..fa6b560 100644
> --- a/bfd/config.bfd
> +++ b/bfd/config.bfd
> @@ -1030,7 +1030,7 @@ case "${targ}" in
>      targ_defvec=mips_elf32_trad_le_vec
>      targ_selvecs="mips_elf32_trad_be_vec mips_elf32_ntrad_be_vec mips_elf32_ntrad_le_vec mips_elf64_trad_be_vec mips_elf64_trad_le_vec"
>      ;;
> -  mips*-sde-elf* | mips*-mti-elf*)
> +  mips*-sde-elf* | mips*-mti-elf* | mips*-img-elf*)
>      targ_defvec=mips_elf32_trad_be_vec
>      targ_selvecs="mips_elf32_trad_le_vec mips_elf32_ntrad_be_vec mips_elf32_ntrad_le_vec mips_elf64_trad_be_vec mips_elf64_trad_le_vec"
>      ;;
> diff --git a/configure b/configure
> index 3645571..30b2a50 100755
> --- a/configure
> +++ b/configure
> @@ -3778,7 +3778,7 @@ case "${target}" in
>    microblaze*)
>      noconfigdirs="$noconfigdirs gprof"
>      ;;
> -  mips*-sde-elf* | mips*-mti-elf*)
> +  mips*-sde-elf* | mips*-mti-elf* | mips*-img-elf*)
>      if test x$with_newlib = xyes; then
>        noconfigdirs="$noconfigdirs gprof"
>      fi
> @@ -6983,7 +6983,7 @@ case "${target}" in
>    spu-*-*)
>      target_makefile_frag="config/mt-spu"
>      ;;
> -  mips*-sde-elf* | mips*-mti-elf*)
> +  mips*-sde-elf* | mips*-mti-elf* | mips*-img-elf*)
>      target_makefile_frag="config/mt-sde"
>      ;;
>    mipsisa*-*-elfoabi*)
> diff --git a/configure.ac b/configure.ac
> index 07c3a66..f1d8d0c 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -1106,7 +1106,7 @@ case "${target}" in
>    microblaze*)
>      noconfigdirs="$noconfigdirs gprof"
>      ;;
> -  mips*-sde-elf* | mips*-mti-elf*)
> +  mips*-sde-elf* | mips*-mti-elf* | mips*-img-elf*)
>      if test x$with_newlib = xyes; then
>        noconfigdirs="$noconfigdirs gprof"
>      fi
> @@ -2361,7 +2361,7 @@ case "${target}" in
>    spu-*-*)
>      target_makefile_frag="config/mt-spu"
>      ;;
> -  mips*-sde-elf* | mips*-mti-elf*)
> +  mips*-sde-elf* | mips*-mti-elf* | mips*-img-elf*)
>      target_makefile_frag="config/mt-sde"
>      ;;
>    mipsisa*-*-elfoabi*)
> diff --git a/gas/configure.tgt b/gas/configure.tgt
> index 7d5afa9..11f2688 100644
> --- a/gas/configure.tgt
> +++ b/gas/configure.tgt
> @@ -326,7 +326,8 @@ case ${generic_target} in
>    mips*-*-freebsd* | mips*-*-kfreebsd*-gnu)
>  					fmt=elf em=freebsd ;;
>    mips-*-sysv4*MP* | mips-*-gnu*)	fmt=elf em=tmips ;;
> -  mips*-sde-elf* | mips*-mti-elf*)	fmt=elf em=tmips ;;
> +  mips*-sde-elf* | mips*-mti-elf* | mips*-img-elf*)
> +					fmt=elf em=tmips ;;
>    mips-*-elf* | mips-*-rtems*)		fmt=elf ;;
>    mips-*-netbsd*)			fmt=elf em=tmips ;;
>    mips-*-openbsd*)			fmt=elf em=tmips ;;
> diff --git a/ld/configure.tgt b/ld/configure.tgt
> index 0eb743d..133ded3 100644
> --- a/ld/configure.tgt
> +++ b/ld/configure.tgt
> @@ -454,7 +454,7 @@ mips*vr5000el-*-elf*)	targ_emul=elf32l4300 ;;
>  mips*vr5000-*-elf*)	targ_emul=elf32b4300 ;;
>  mips*el-sde-elf*)	targ_emul=elf32ltsmip
>  			targ_extra_emuls="elf32btsmip elf32ltsmipn32 elf64ltsmip elf32btsmipn32 elf64btsmip" ;;
> -mips*-sde-elf* | mips*-mti-elf*)
> +mips*-sde-elf* | mips*-mti-elf* | mips*-img-elf*)
>  			targ_emul=elf32btsmip
>  			targ_extra_emuls="elf32ltsmip elf32btsmipn32 elf64btsmip elf32ltsmipn32 elf64ltsmip" ;;
>  mips64*el-ps2-elf*)	targ_emul=elf32lr5900n32
> diff --git a/ld/testsuite/ld-mips-elf/mips-elf.exp b/ld/testsuite/ld-mips-elf/mips-elf.exp
> index a2632b2..2d3f052 100644
> --- a/ld/testsuite/ld-mips-elf/mips-elf.exp
> +++ b/ld/testsuite/ld-mips-elf/mips-elf.exp
> @@ -55,7 +55,8 @@ if {![istarget mips*-*-*] || ![is_elf_format]} {
>  set has_newabi [expr [istarget *-*-irix6*] \
>  		     || [istarget mips*-*-linux*] \
>  		     || [istarget mips*-sde-elf*] \
> -		     || [istarget mips*-mti-elf*]]
> +		     || [istarget mips*-mti-elf*] \
> +		     || [istarget mips*-img-elf*]]
>  set linux_gnu [expr [istarget mips*-*-linux*]]
>  set embedded_elf [expr [istarget mips*-*-elf]]
> @@ -79,7 +80,7 @@ if { [istarget *-*-irix6*] } {
>      set abi_ldflags(o32) -melf32btsmip_fbsd
>  }
>  if { [istarget mips*-*-linux*] || [istarget mips*-sde-elf*]
> -     || [istarget mips*-mti-elf*] } {
> +     || [istarget mips*-mti-elf*] || [istarget mips*-img-elf*]} {
>      set abi_ldflags(n32) -melf32btsmipn32
>      set abi_ldflags(n64) -melf64btsmip
>  } elseif { [istarget mips64*-*freebsd*] } {

Please add these parts to 0001 rather than 0002.  0001 is OK with
that change, thanks.

> +  HOWTO (R_MIPS_PCLO16,		/* type */
> +	 0,			/* rightshift */
> +	 2,			/* size (0 = byte, 1 = short, 2 = long) */
> +	 16,			/* bitsize */
> +	 TRUE,			/* pc_relative */
> +	 0,			/* bitpos */
> +	 complain_overflow_signed, /* complain_on_overflow */
> +	 _bfd_mips_elf_generic_reloc,   /* special_function */
> +	 "R_MIPS_PCLO16",	/* name */
> +	 TRUE,			/* partial_inplace */
> +	 0x0000ffff,		/* src_mask */
> +	 0x0000ffff,		/* dst_mask */
> +	 TRUE),			/* pcrel_offset */
> +
>  };

I think this should be complain_overflow_dont.

Nit: no blank line at end of brace list.

> @@ -1088,6 +1093,14 @@ static const bfd_vma mips_exec_plt_entry[] =
>    0x03200008	/* jr $25					*/
>  };
 
> +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					*/
> +};
> +
>  /* The format of subsequent MIPS16 o32 PLT entries.  We use v0 ($2)
>     and v1 ($3) as temporaries because t8 ($24) and t9 ($25) are not
>     directly addressable.  */

Might be worth adding a comment saying that the JR and ADDIU will
be swapped due to LOAD_INTERLOCKS_P being true.

>      case R_MIPS_PC16:
>      case R_MIPS_GNU_REL16_S2:
> -      value = symbol + _bfd_mips_elf_sign_extend (addend, 18) - p;
> +      if (howto->partial_inplace)
> +	addend = _bfd_mips_elf_sign_extend (addend, 18);
> +      value = symbol + addend - p;
>        overflowed_p = mips_elf_overflow_p (value, 18);
>        value >>= howto->rightshift;
>        value &= howto->dst_mask;
>        break;
>
> +    case R_MIPS_PC21_S2:
> +      if (howto->partial_inplace)
> +	addend = _bfd_mips_elf_sign_extend (addend, 23);
> +      value = symbol + addend - p;
> +      overflowed_p = mips_elf_overflow_p (value, 23);
> +      value >>= howto->rightshift;
> +      value &= howto->dst_mask;
> +      break;
> +
> +    case R_MIPS_PC26_S2:
> +      if (howto->partial_inplace)
> +	addend = _bfd_mips_elf_sign_extend (addend, 28);
> +      value = symbol + addend - p;
> +      overflowed_p = mips_elf_overflow_p (value, 28);
> +      value >>= howto->rightshift;
> +      value &= howto->dst_mask;
> +      break;
> +
> +    case R_MIPS_PC18_S3:
> +      if (howto->partial_inplace)
> +	addend = _bfd_mips_elf_sign_extend (addend, 21);
> +
> +      if ((symbol + addend) & 7)
> +	return bfd_reloc_outofrange;
> +
> +      value = symbol + addend - ((p | 7) ^ 7);
> +      overflowed_p = mips_elf_overflow_p (value, 21);
> +      value >>= howto->rightshift;
> +      value &= howto->dst_mask;
> +      break;
> +
> +    case R_MIPS_PC19_S2:
> +      if (howto->partial_inplace)
> +	addend = _bfd_mips_elf_sign_extend (addend, 21);
> +
> +      if ((symbol + addend) & 3)
> +	return bfd_reloc_outofrange;
> +
> +      value = symbol + addend - p;
> +      overflowed_p = mips_elf_overflow_p (value, 21);
> +      value >>= howto->rightshift;
> +      value &= howto->dst_mask;
> +      break;

I think we should be consistent and check the low 2 bits for all S2s
or none.  Probably all is better (including R_MIPS_GNU_REL16_S2).
I realise it should never happen for insn-to-insn references, but still.

> @@ -1592,55 +1631,73 @@ struct mips_ase
>    int mips64_rev;
>    int micromips32_rev;
>    int micromips64_rev;
> +
> +  /* The architecture revisions for MIPS32, MIPS64, microMIPS32 and microMIPS64
> +     where the ASE was removed or -1 if the extension has not been removed.  */
> +  int mips32_rem_rev;
> +  int mips64_rem_rev;
> +  int micromips32_rem_rev;
> +  int micromips64_rem_rev;
>  };

Sorry, I should have flagged this up last time, but wouldn't it be
better to have just one "rem_rev" field here?  AFAIK there's no
intention of removing ASEs from 32rN but keeping them for 64rN, or
removing them from the standard encoding but keeping them for microMIPS.

With that change, this:

> @@ -1953,6 +2021,16 @@ mips_check_isa_supports_ase (const struct mips_ase *ase)
>  	as_warn (_("the `%s' extension requires %s%d revision %d or greater"),
>  		 ase->name, base, size, min_rev);
>      }
> +  if ((rem_rev > 0 && mips_isa_rev () >= rem_rev)
> +      && (warned_isa & ase->flags) != ase->flags)
> +    {
> +      warned_isa |= ase->flags;
> +      base = mips_opts.micromips ? "microMIPS" : "MIPS";
> +      size = ISA_HAS_64BIT_REGS (mips_opts.isa) ? 64 : 32;
> +      as_warn (_("the `%s' extension was removed in %s%d revision %d"),
> +	       ase->name, base, size, rem_rev);
> +    }
> +
>    if ((ase->flags & FP64_ASES)
>        && mips_opts.fp != 64
>        && (warned_fp32 & ase->flags) != ase->flags)

would become an "else".

> @@ -3665,6 +3743,8 @@ mips_check_options (struct mips_set_options *opts, bfd_boolean abi_checks)
>        if (abi_checks
>  	  && ABI_NEEDS_64BIT_REGS (mips_abi))
>  	as_warn (_("`fp=32' used with a 64-bit ABI"));
> +      if (ISA_IS_R6 (mips_opts.isa))
> +	as_bad (_("`fp=32' used with a MIPS R6 cpu"));
>        break;
>      default:
>        as_bad (_("Unknown size of floating point registers"));
[...]
> @@ -3721,6 +3810,9 @@ file_mips_check_options (void)
>  	       && ISA_HAS_64BIT_FPRS (file_mips_opts.isa))
>  	/* Handle ASEs that require 64-bit float registers, if possible.  */
>  	file_mips_opts.fp = 64;
> +      else if (ISA_IS_R6 (mips_opts.isa))
> +	/* R6 implies 64-bit float registers.  */
> +	file_mips_opts.fp = 64;
>        else
>  	/* 32-bit float registers.  */
>  	file_mips_opts.fp = 32;

When this came up in GCC, it sounded like r6 does support -mfp32 for
-msingle-float (but not otherwise).

> @@ -3673,6 +3753,15 @@ mips_check_options (struct mips_set_options *opts, bfd_boolean abi_checks)
 
>    if (opts->micromips == 1 && opts->mips16 == 1)
>      as_bad (_("`mips16' cannot be used with `micromips'"));
> +  else if (ISA_IS_R6 (mips_opts.isa)
> +	   && (opts->micromips == 1
> +	       || opts->mips16 == 1))
> +    as_fatal (_("neither `micromips' nor `mips16' can be used with "
> +		"`mips32r6' or `mips64r6'"));
> +
> +  if (ISA_IS_R6 (opts->isa) && mips_relax_branch)
> +    as_fatal (_("branch relaxation is not supported in `mips32r6' "
> +		"or `mips64r6'"));
>  }
> 
>  /* Perform consistency checks on the module level options exactly once.

Please make the errors specific: we know whether it's micromips or mips16,
and whether it's mips32r6 or mips64r6.

> @@ -3748,6 +3840,11 @@ file_mips_check_options (void)
>      file_mips_opts.micromips = (CPU_HAS_MICROMIPS (file_mips_opts.arch))
>  				? 1 : 0;
> 
> +  if (mips_nan2008 == -1)
> +    mips_nan2008 = (ISA_HAS_LEGACY_NAN (file_mips_opts.isa)) ? 0 : 1;
> +  else if (!ISA_HAS_LEGACY_NAN (file_mips_opts.isa) && mips_nan2008 == 0)
> +    as_fatal (_("current isa does not support legacy NaN"));
> +
>    /* Some ASEs require 64-bit FPRs, so -mfp32 should stop those ASEs from
>       being selected implicitly.  */
>    if (file_mips_opts.fp != 64)

current isa == mips_cpu_info_from_arch (file_mips_opts.arch)->name

> +  if (operand->check_not_zero && regno == 0)
> +    {
> +      set_insn_error (arg->argnum, _("the source register must not be $0"));
> +      return FALSE;
> +    }
> +
> +  if (operand->check_not_zero && operand->check_not_equal
> +      && regno == 0 && regno == arg->last_regno)
> +    {
> +      set_insn_error (arg->argnum,
> +                      _("the source registers must not be $0 and different"));
> +      return FALSE;
> +    }
> +

This doesn't look right: the second condition can never trigger after
the first.  With this and:

> +  if (operand->check_greater_than && regno <= arg->last_regno)
> +    return FALSE;
> +  else if (operand->check_less_than && regno >= arg->last_regno)
> +    return FALSE;
> +  else if (operand->check_greater_than_or_equal && regno < arg->last_regno)
> +    return FALSE;
> +  else if (operand->check_less_than_or_equal && regno > arg->last_regno)
> +    return FALSE;

...this I think it would be simpler to have three fields: less_than_ok,
equal_ok and greater_than_ok, with more than one being set where necessary.
Probably also zero_ok for consistency.

> @@ -5680,6 +5862,13 @@ match_operand (struct mips_arg_info *arg,
 
>      case OP_REG_INDEX:
>        return match_reg_index_operand (arg, operand);
> +
> +    case OP_SAME_RS_RT:
> +      return match_same_rs_rt_operand (arg, operand);
> +
> +    case OP_CHECK_PREV:
> +      return match_check_prev_operand (arg, operand);
> +
>      }

No blank line before "}".

> +    case BFD_RELOC_MIPS_18_PCREL_S3:
> +      if ((*valP & 0x7) != 0)
> +	as_bad_where (fixP->fx_file, fixP->fx_line,
> +		      _("pc rel from misaligned address (%lx)"),
> +		      (long) *valP);
> +
> +      gas_assert(!fixP->fx_done);
> +      break;
> +
> +    case BFD_RELOC_MIPS_19_PCREL_S2:
> +      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;

"PC-relative access to misaligned address", for consistency with
"branch to misaligned address"?

Nit: space after gas_assert.

> @@ -15856,10 +16181,15 @@ s_nan (int ignore ATTRIBUTE_UNUSED)
 
>    if (i == sizeof (str_2008) - 1
>        && memcmp (input_line_pointer, str_2008, i) == 0)
> -    mips_flag_nan2008 = TRUE;
> +    mips_nan2008 = 1;
>    else if (i == sizeof (str_legacy) - 1
>  	   && memcmp (input_line_pointer, str_legacy, i) == 0)
> -    mips_flag_nan2008 = FALSE;
> +    {
> +      if (ISA_HAS_LEGACY_NAN (file_mips_opts.isa))
> +	mips_nan2008 = 0;
> +      else
> +	as_fatal (_("current isa does not support legacy NaN"));
> +    }
>    else
>      as_bad (_("bad .nan directive"));

Same "current isa" comment as above.

> diff --git a/gas/testsuite/gas/mips/ld-zero-3.s b/gas/testsuite/gas/mips/ld-zero-3.s
> index 7ca414c..15d62ed 100644
> --- a/gas/testsuite/gas/mips/ld-zero-3.s
> +++ b/gas/testsuite/gas/mips/ld-zero-3.s
> @@ -2,7 +2,9 @@
>  foo:
>  	lwu	$0, 0x12345678($2)
>  	ld	$0, 0x12345678($2)
> +.ifndef r6
>  	lld	$0, 0x12345678($2)
> +.endif
> 
>  # Force some (non-delay-slot) zero bytes, to make 'objdump' print ...
>  	.align	4, 0
> @@ -653,7 +680,7 @@ if { [istarget mips*-*-vxworks*] } {
>  					[mips_arch_list_matching mips3 !singlefloat]
>      }
>      run_dump_test_arches "ld-zero"	[mips_arch_list_matching mips1]
> -    run_dump_test_arches "ld-zero-2"	[mips_arch_list_matching mips2 !nollsc]
> +    run_dump_test_arches "ld-zero-2"	[mips_arch_list_matching mips2 !nollsc !mips32r6]
>      run_dump_test_arches "ld-zero-3"	[mips_arch_list_matching mips3 !nollsc]
>      run_dump_test_arches "ld-zero-u"	[mips_arch_list_matching micromips]
>      run_dump_test_arches "ld-zero-q"	[mips_arch_list_matching r5900]

Until now the macro code has been updated to handle reduced-range offsets,
so that "lld $0, 0x12345678($2)" would still work.  It would mean extending:

    case M_LL_AB:
      s = "ll";
      fmt = MEM12_FMT;
      offbits = (mips_opts.micromips ? 12 : 16);
      goto ld;
    case M_LLD_AB:
      s = "lld";
      fmt = MEM12_FMT;
      offbits = (mips_opts.micromips ? 12 : 16);
      goto ld;

to handle the r6 case.

On the other hand, macros are an abomination so personally I wouldn't
mind if we say that they're deprecated for r6 and later.

> @@ -1206,4 +1233,9 @@ if { [istarget mips*-*-vxworks*] } {
>      run_dump_test "module-override"
>      run_dump_test "module-defer-warn1"
>      run_list_test "module-defer-warn2" -32
> +
> +    run_dump_test_arches "r6"		[mips_arch_list_matching mips32r6 !micromips]
> +    run_dump_test_arches "r6-64"	[mips_arch_list_matching mips64r6 !micromips]
> +    run_list_test_arches "r6-removed"	[mips_arch_list_matching mips32r6]
> +    run_list_test_arches "r6-64-removed"	[mips_arch_list_matching mips64r6]
>  }

Are the "!micromips"es here needed?  I think it'd be more robust to drop
them if they're not doing anything at the moment

> @@ -1368,8 +1437,7 @@ print_insn_arg (struct disassemble_info *info,
>        break;
>
>      case OP_REPEAT_DEST_REG:
> -      /* Should always match OP_REPEAT_PREV_REG first.  */
> -      abort ();
> +      print_reg (info, opcode, state->last_reg_type, state->dest_regno);
> 
>      case OP_PC:
>        infprintf (is, "$pc");

Missing break, although rather than have:

>  /* Print operand OPERAND of OPCODE, using STATE to track inter-operand state.
>     UVAL is the encoding of the operand (shifted into bit 0) and BASE_PC is
>     the base address for OP_PCREL operands.  */
> 
> -static void
> +static bfd_boolean
>  print_insn_arg (struct disassemble_info *info,
>  		struct mips_print_arg_state *state,
>  		const struct mips_opcode *opcode,

I think it would be better to have a separate function that checks
these constraints first.  Something like:

  init_print_arg_state (&state);
  for (s = opcode->args; *s; ++s)
    {
      operand = decode_operand (s);
      if (operand)
        switch (operand->type)
          {
          case OP_REG:
          case OP_OPTIONAL_REG:
            {
              const struct mips_reg_operand *reg_op;

              reg_op = (const struct mips_reg_operand *) operand;
              uval = mips_decode_reg_operand (reg_op, uval);
              mips_seen_register (state, uval, reg_op->reg_type);
            }
            break;

          ...your new checks here...
          }
      ...move onto next char...
    }
  return TRUE;

That would avoid having to use a temporary buffer.

Looks good otherwise, thanks.

Richard