Re: [PATCH 1/2] Add support for O32 FPXX ABI

[Richard Sandiford <rdsandiford at googlemail dot com>]

Thanks for all the work, mostly looks good to me FWIW.

Matthew Fortune <Matthew.Fortune@imgtec.com> writes:
> +/* Return true if the given ELF header flags describe a 32-bit binary.  */
> +
> +static bfd_boolean
> +mips_32bit_flags_p (flagword flags)
> +{
> +  return ((flags & EF_MIPS_32BITMODE) != 0
> +	  || (flags & EF_MIPS_ABI) == E_MIPS_ABI_O32
> +	  || (flags & EF_MIPS_ABI) == E_MIPS_ABI_EABI32
> +	  || (flags & EF_MIPS_ARCH) == E_MIPS_ARCH_1
> +	  || (flags & EF_MIPS_ARCH) == E_MIPS_ARCH_2
> +	  || (flags & EF_MIPS_ARCH) == E_MIPS_ARCH_32
> +	  || (flags & EF_MIPS_ARCH) == E_MIPS_ARCH_32R2);
> +}

OK, this is preexisting code, but I'm not sure it's right.
32bitness should be taken purely from the ABI, not the architecture,
otherwise we could treat 32-bit MIPS64r2 code differently from MIPS32r2
code.

Since o32 was the original (with no flags), I think we should have
mips_64bit_flags_p instead.

OTOH that should be done separately from your patch, so never mind.

> +/* Infer the content of the ABI flags based on the elf header.  */
> +
> +static void
> +infer_mips_abiflags (bfd *abfd, Elf_Internal_ABIFlags_v0* abiflags)
> +{
> +  obj_attribute *in_attr;
> +

Would feel more comfortable with a clearing memset here, and leave
the explicit 0s out.

> +  abiflags->version = 0;
> +
> +  update_mips_abiflags_isa (abfd, abiflags);
> +
> +  if (mips_32bit_flags_p (elf_elfheader (abfd)->e_flags))
> +    abiflags->gpr_size = AFL_REG_32;
> +  else
> +    abiflags->gpr_size = AFL_REG_64;
> +
> +  abiflags->cpr1_size = AFL_REG_NONE;
> +
> +  in_attr = elf_known_obj_attributes (abfd)[OBJ_ATTR_GNU];
> +  abiflags->fp_abi = in_attr[Tag_GNU_MIPS_ABI_FP].i;
> +
> +  if (abiflags->fp_abi == Val_GNU_MIPS_ABI_FP_SINGLE
> +      || abiflags->fp_abi == Val_GNU_MIPS_ABI_FP_XX
> +      || (abiflags->fp_abi == Val_GNU_MIPS_ABI_FP_DOUBLE
> +	  && abiflags->gpr_size == AFL_REG_32))
> +    abiflags->cpr1_size = AFL_REG_32;
> +  else if (abiflags->fp_abi == Val_GNU_MIPS_ABI_FP_DOUBLE
> +	   || abiflags->fp_abi == Val_GNU_MIPS_ABI_FP_64)
> +    abiflags->cpr1_size = AFL_REG_64;
> +
> +  abiflags->cpr2_size = AFL_REG_NONE;
> +
> +  abiflags->isa_ext = 0;

This can be set from EF_MIPS_MACH, at least to some extent.

> @@ -13665,12 +13887,47 @@ _bfd_mips_elf_final_link (bfd *abfd, struct bfd_link_info *info)
>  
>    /* Go through the sections and collect the .reginfo and .mdebug
>       information.  */
> +  abiflags_sec = NULL;
>    reginfo_sec = NULL;
>    mdebug_sec = NULL;
>    gptab_data_sec = NULL;
>    gptab_bss_sec = NULL;
>    for (o = abfd->sections; o != NULL; o = o->next)
>      {
> +      if (strcmp (o->name, ".MIPS.abiflags") == 0)
> +	{
> +	  /* We have found the .MIPS.abiflags section in the output file.
> +	     Look through all the link_orders comprising it and remove them.
> +	     The data is merged in _bfd_mips_elf_merge_private_bfd_data.  */
> +	  for (p = o->map_head.link_order; p != NULL; p = p->next)
> +	    {
> +	      asection *input_section;
> +
> +	      if (p->type != bfd_indirect_link_order)
> +		{
> +		  if (p->type == bfd_data_link_order)
> +		    continue;
> +		  abort ();
> +		}
> +
> +	      input_section = p->u.indirect.section;
> +
> +	      /* Hack: reset the SEC_HAS_CONTENTS flag so that
> +		 elf_link_input_bfd ignores this section.  */
> +	      input_section->flags &= ~SEC_HAS_CONTENTS;
> +	    }
> +
> +	  /* Size has been set in _bfd_mips_elf_always_size_sections.  */
> +	  BFD_ASSERT(o->size == sizeof (Elf_External_ABIFlags_v0));
> +
> +	  /* Skip this section later on (I don't think this currently
> +	     matters, but someday it might).  */
> +	  o->map_head.link_order = NULL;
> +
> +	  abiflags_sec = o;
> +
> +	}

Excess blank line.

> @@ -14155,6 +14412,24 @@ _bfd_mips_elf_final_link (bfd *abfd, struct bfd_link_info *info)
>  
>    /* Now write out the computed sections.  */
>  
> +  if (abiflags_sec != NULL)
> +    {
> +      Elf_External_ABIFlags_v0 ext;
> +      Elf_Internal_ABIFlags_v0 *abiflags;
> +
> +      abiflags = &mips_elf_tdata (abfd)->abiflags;
> +
> +      /* Set up the abiflags if no valid input sections were found.  */
> +      if (!mips_elf_tdata (abfd)->abiflags_valid)
> +	{
> +	  infer_mips_abiflags (abfd, abiflags);
> +	  mips_elf_tdata (abfd)->abiflags_valid = TRUE;
> +	}

I imagine this is right, but which case does it handle?  Links with
no MIPS ELF inputs?

> @@ -14688,6 +15083,86 @@ _bfd_mips_elf_merge_private_bfd_data (bfd *ibfd, bfd *obfd)
>    if (null_input_bfd)
>      return TRUE;
>  
> +  /* Populate abiflags using existing information.  */
> +  if (!mips_elf_tdata (ibfd)->abiflags_valid)
> +    {
> +      infer_mips_abiflags (ibfd, &mips_elf_tdata (ibfd)->abiflags);
> +      mips_elf_tdata (ibfd)->abiflags_valid = TRUE;
> +    }
> +  else
> +    {
> +      Elf_Internal_ABIFlags_v0 abiflags;
> +      Elf_Internal_ABIFlags_v0 *iabiflags;
> +      infer_mips_abiflags (ibfd, &abiflags);
> +      iabiflags = &mips_elf_tdata (ibfd)->abiflags;
> +
> +      if (iabiflags->isa_level != abiflags.isa_level
> +	  || iabiflags->isa_rev != abiflags.isa_rev)
> +	(*_bfd_error_handler)
> +	  (_("%B: warning: Inconsistent ISA between e_flags and "
> +	     ".MIPS.abiflags"), ibfd);
> +      if (abiflags.fp_abi != Val_GNU_MIPS_ABI_FP_ANY
> +	  && iabiflags->fp_abi != abiflags.fp_abi)
> +	(*_bfd_error_handler)
> +	  (_("%B: warning: Inconsistent FP ABI between e_flags and "
> +	     ".MIPS.abiflags"), ibfd);
> +      if ((iabiflags->ases & abiflags.ases) != abiflags.ases)
> +	(*_bfd_error_handler)
> +	  (_("%B: warning: Inconsistent ASEs between e_flags and "
> +	     ".MIPS.abiflags"), ibfd);

Minor nit, but "abiflags" vs. "iabiflags" is a bit hard to follow,
since the "i" could be "implicit" (but I assume is "input").

> +  if (!mips_elf_tdata (obfd)->abiflags_valid)
> +    {
> +      /* Copy input abiflags if output abiflags are not already valid.  */
> +      mips_elf_tdata (obfd)->abiflags = mips_elf_tdata (ibfd)->abiflags;
> +      mips_elf_tdata (obfd)->abiflags_valid = TRUE;
> +    }
> +
> +  if (! elf_flags_init (obfd))
> +    {
> +      elf_flags_init (obfd) = TRUE;
> +      elf_elfheader (obfd)->e_flags = new_flags;
> +      elf_elfheader (obfd)->e_ident[EI_CLASS]
> +	= elf_elfheader (ibfd)->e_ident[EI_CLASS];
> +
> +      if (bfd_get_arch (obfd) == bfd_get_arch (ibfd)
> +	  && (bfd_get_arch_info (obfd)->the_default
> +	      || mips_mach_extends_p (bfd_get_mach (obfd),
> +				      bfd_get_mach (ibfd))))
> +	{
> +	  if (! bfd_set_arch_mach (obfd, bfd_get_arch (ibfd),
> +				   bfd_get_mach (ibfd)))
> +	    return FALSE;
> +	}
> +
> +      return TRUE;
> +    }
> +
> +  /* Update the output abiflags fp_abi using the computed fp_abi.  */
> +  obj_attribute *out_attr = elf_known_obj_attributes (obfd)[OBJ_ATTR_GNU];
> +  mips_elf_tdata (obfd)->abiflags.fp_abi = out_attr[Tag_GNU_MIPS_ABI_FP].i;
> +
> +#define max(a,b) ((a) > (b) ? (a) : (b))
> +  /* Merge abiflags.  */
> +  mips_elf_tdata (obfd)->abiflags.gpr_size
> +    = max (mips_elf_tdata (obfd)->abiflags.gpr_size,
> +	   mips_elf_tdata (ibfd)->abiflags.gpr_size);
> +  mips_elf_tdata (obfd)->abiflags.cpr1_size
> +    = max (mips_elf_tdata (obfd)->abiflags.cpr1_size,
> +	   mips_elf_tdata (ibfd)->abiflags.cpr1_size);
> +  mips_elf_tdata (obfd)->abiflags.cpr2_size
> +    = max (mips_elf_tdata (obfd)->abiflags.cpr2_size,
> +	   mips_elf_tdata (ibfd)->abiflags.cpr2_size);
> +#undef max
> +  mips_elf_tdata (obfd)->abiflags.ases
> +    |= mips_elf_tdata (ibfd)->abiflags.ases;
> +  mips_elf_tdata (obfd)->abiflags.isa_ext
> +    |= mips_elf_tdata (ibfd)->abiflags.isa_ext;

isa_ext should be handled by bfd_set_arch_mach.

> @@ -14944,6 +15436,130 @@ _bfd_mips_elf_get_target_dtag (bfd_vma dtag)
>      }
>  }
>  
> +static void
> +print_mips_ases (FILE *file, unsigned int mask)
> +{
> +  if (mask & AFL_ASE_DSP)
> +    fputs ("\n\tDSP ASE", file);
> +  if (mask & AFL_ASE_DSP64)
> +    fputs ("\n\tDSP ASE (64-bit)", file);
> +  if (mask & AFL_ASE_DSPR2)
> +    fputs ("\n\tDSP R2 ASE", file);
> +  if (mask & AFL_ASE_EVA)
> +    fputs ("\n\tEnhanced VA Scheme", file);
> +  if (mask & AFL_ASE_MCU)
> +    fputs ("\n\tMCU (MicroController) ASE", file);
> +  if (mask & AFL_ASE_MDMX)
> +    fputs ("\n\tMDMX ASE", file);
> +  if (mask & AFL_ASE_MIPS3D)
> +    fputs ("\n\tMIPS-3D ASE", file);
> +  if (mask & AFL_ASE_MT)
> +    fputs ("\n\tMT ASE", file);
> +  if (mask & AFL_ASE_SMARTMIPS)
> +    fputs ("\n\tSmartMIPS ASE", file);
> +  if (mask & AFL_ASE_VIRT)
> +    fputs ("\n\tVZ ASE", file);
> +  if (mask & AFL_ASE_VIRT64)
> +    fputs ("\n\tVZ ASE (64-bit)", file);
> +  if (mask & AFL_ASE_MSA)
> +    fputs ("\n\tMSA ASE", file);
> +  if (mask & AFL_ASE_MSA64)
> +    fputs ("\n\tMSA ASE (64-bit)", file);
> +  if (mask & AFL_ASE_MIPS16)
> +    fputs ("\n\tMIPS16 ASE", file);
> +  if (mask & AFL_ASE_MICROMIPS)
> +    fputs ("\n\tMICROMIPS ASE", file);
> +  if (mask & AFL_ASE_XPA)
> +    fputs ("\n\tXPA ASE", file);
> +  if (mask == 0)
> +    fputs ("\n\tNone", file);

Hmm, wasn't the plan to get rid of the 64-bit versions?  I think the external
interface really shouldn't have this distinction.  If we can do that while
still using the AFL_* numbering internally then great, but if something
has to give, it should be having a different external and internal numbering.

> +static void
> +print_mips_isa_ext (FILE *file, unsigned int mask)
> +{
> +  if (mask & AFL_EXT_XLR)
> +    fputs ("\n\tRMI Xlr instruction", file);
> +  if (mask & AFL_EXT_OCTEON2)
> +    fputs ("\n\tCavium Networks Octeon2", file);
> +  if (mask & AFL_EXT_OCTEONP)
> +    fputs ("\n\tCavium Networks OcteonP", file);
> +  if (mask & AFL_EXT_LOONGSON_3A)
> +    fputs ("\n\tLoongson 3A", file);
> +  if (mask & AFL_EXT_OCTEON)
> +    fputs ("\n\tCavium Networks Octeon", file);
> +  if (mask & AFL_EXT_5900)
> +    fputs ("\n\tMIPS R5900", file);
> +  if (mask & AFL_EXT_4650)
> +    fputs ("\n\tMIPS R4650", file);
> +  if (mask & AFL_EXT_4010)
> +    fputs ("\n\tLSI R4010", file);
> +  if (mask & AFL_EXT_4100)
> +    fputs ("\n\tNEC VR4100", file);
> +  if (mask & AFL_EXT_3900)
> +    fputs ("\n\tToshiba R3900", file);
> +  if (mask & AFL_EXT_10000)
> +    fputs ("\n\tMIPS R10000", file);
> +  if (mask & AFL_EXT_SB1)
> +    fputs ("\n\tBroadcom SB-1", file);
> +  if (mask & AFL_EXT_4111)
> +    fputs ("\n\tNEC VR4111/VR4181", file);
> +  if (mask & AFL_EXT_4120)
> +    fputs ("\n\tNEC VR4120", file);
> +  if (mask & AFL_EXT_5400)
> +    fputs ("\n\tNEC VR5400", file);
> +  if (mask & AFL_EXT_5500)
> +    fputs ("\n\tNEC VR5500", file);
> +  if (mask & AFL_EXT_LOONGSON_2E)
> +    fputs ("\n\tST Microelectronics Loongson 2E", file);
> +  if (mask & AFL_EXT_LOONGSON_2F)
> +    fputs ("\n\tST Microelectronics Loongson 2F", file);
> +  if (mask == 0)
> +    fputs ("\n\tNone", file);

As I said before, I think this should be an enum rather than a bitmask.

> +/* This is the struct we use to hold the module level set of options.  Note
> +   that we must set the isa field to ISA_UNKNOWN and the ASE fields to
> +   -1 to indicate that they have not been initialized.  */
>  
> -/* 1 if -msingle-float, 0 if -mdouble-float.  The default is 0.   */
> -static int file_mips_single_float = 0;
> +static struct mips_set_options file_mips_opts =
> +{
> +  /* isa */ ISA_UNKNOWN, /* ase */ 0, /* mips16 */ -1, /* micromips */ -1,
> +  /* noreorder */ 0,  /* at */ ATREG, /* warn_about_macros */ 0,
> +  /* nomove */ 0, /* nobopt */ 0, /* noautoextend */ 0, /* insn32 */ FALSE,
> +  /* gp32 */ -1, /* fp */ -1, /* arch */ CPU_UNKNOWN, /* sym32 */ FALSE,
> +  /* soft_float */ FALSE, /* single_float */ FALSE,
> +  /* mips_defer_fp_warn */ FALSE
> +};
>  
> -/* True if -mnan=2008, false if -mnan=legacy.  */
> -static bfd_boolean mips_flag_nan2008 = FALSE;
> +/* This is the struct we use to hold the current set of options.  Note
> +   that we must set the isa field to ISA_UNKNOWN and the ASE fields to
> +   -1 to indicate that they have not been initialized.  */
>  
>  static struct mips_set_options mips_opts =
>  {
>    /* isa */ ISA_UNKNOWN, /* ase */ 0, /* mips16 */ -1, /* micromips */ -1,
>    /* noreorder */ 0,  /* at */ ATREG, /* warn_about_macros */ 0,
>    /* nomove */ 0, /* nobopt */ 0, /* noautoextend */ 0, /* insn32 */ FALSE,
> -  /* gp32 */ 0, /* fp32 */ 0, /* arch */ CPU_UNKNOWN, /* sym32 */ FALSE,
> -  /* soft_float */ FALSE, /* single_float */ FALSE
> +  /* gp32 */ -1, /* fp */ -1, /* arch */ CPU_UNKNOWN, /* sym32 */ FALSE,
> +  /* soft_float */ FALSE, /* single_float */ FALSE,
> +  /* mips_defer_fp_warn */ FALSE
>  };

Let's not duplicate the comments like this -- please fuse them together
more naturally.

> @@ -3602,6 +3627,173 @@ md_mips_end (void)
>    mips_emit_delays ();
>    if (! ECOFF_DEBUGGING)
>      md_obj_end ();
> +
> +  int fpabi = bfd_elf_get_obj_attr_int (stdoutput, OBJ_ATTR_GNU,
> +					    Tag_GNU_MIPS_ABI_FP);
> +  switch (fpabi)
> +    {
> +    case Val_GNU_MIPS_ABI_FP_DOUBLE:
> +      if ((file_mips_opts.gp32 ? 32 : 64) != file_mips_opts.fp
> +	  || file_mips_opts.single_float == 1
> +	  || file_mips_opts.soft_float == 1)
> +	as_warn (_("Incorrect .gnu_attribute %d,%d found when module is "
> +		   "`%s'"),
> +		 Tag_GNU_MIPS_ABI_FP, fpabi,
> +		 (file_mips_opts.single_float == 1) ? "single-float"
> +		 : (file_mips_opts.soft_float == 1) ? "soft-float"
> +		 : (file_mips_opts.fp == 32) ? "fp32"
> +		 : (file_mips_opts.fp == 64) ? "fp64" : "fpxx");
> +      break;
> +    case Val_GNU_MIPS_ABI_FP_XX:
> +      if (mips_abi != O32_ABI
> +	  || file_mips_opts.fp != 0
> +	  || file_mips_opts.single_float == 1
> +	  || file_mips_opts.soft_float == 1)
> +	as_warn (_("Incorrect .gnu_attribute %d,%d found when module is "
> +		   "`%s'"),
> +		 Tag_GNU_MIPS_ABI_FP, fpabi,
> +		 (mips_abi != O32_ABI) ? "!-mabi=32"
> +		 : (file_mips_opts.single_float == 1) ? "single-float"
> +		 : (file_mips_opts.soft_float == 1) ? "soft-float"
> +		 : (file_mips_opts.fp == 32) ? "fp32"
> +		 : (file_mips_opts.fp == 64) ? "fp64" : "fpxx");
> +      break;
> +    case Val_GNU_MIPS_ABI_FP_64:
> +      if (mips_abi != O32_ABI
> +	  || file_mips_opts.fp != 64
> +	  || file_mips_opts.single_float == 1
> +	  || file_mips_opts.soft_float == 1)
> +	as_warn (_("Incorrect .gnu_attribute %d,%d found when module is "
> +		   "`%s'"),
> +		 Tag_GNU_MIPS_ABI_FP, fpabi,
> +		 (mips_abi != O32_ABI) ? "!-mabi=32"
> +		 : (file_mips_opts.single_float == 1) ? "single-float"
> +		 : (file_mips_opts.soft_float == 1) ? "soft-float"
> +		 : (file_mips_opts.fp == 32) ? "fp32"
> +		 : (file_mips_opts.fp == 64) ? "fp64" : "fpxx");
> +      break;
> +    case Val_GNU_MIPS_ABI_FP_SINGLE:
> +      if (file_mips_opts.single_float != 1)
> +	as_warn (_("Incorrect .gnu_attribute %d,%d found when module is "
> +		   "not `single-float'"),
> +		 Tag_GNU_MIPS_ABI_FP, fpabi);
> +      break;
> +    case Val_GNU_MIPS_ABI_FP_SOFT:
> +      if (file_mips_opts.soft_float != 1)
> +	as_warn (_("Incorrect .gnu_attribute %d,%d found when module is "
> +		   "not `soft-float'"),
> +		 Tag_GNU_MIPS_ABI_FP, fpabi);
> +      break;
> +    case Val_GNU_MIPS_ABI_FP_OLD_64:
> +      as_warn (_("Incorrect .gnu_attribute %d,%d found. ABI not "
> +		 "supported"),
> +	       Tag_GNU_MIPS_ABI_FP, fpabi);
> +      break;
> +    default:
> +      if (fpabi != Val_GNU_MIPS_ABI_FP_ANY)
> +	as_warn (_("Incompatible module option and .gnu_attribute seen, "
> +		   "unexpected Tag_GNU_MIPS_ABI_FP: %d"),
> +		 fpabi);
> +      break;

Warnings should start with lower case (the MIPS code should be consistent
about that now, although the target-independent code isn't always).
Please use %s even where only one incompatibility exists (single and
soft float).  Also, please use the "not `%s'" version when the ABI
wasn't o32 but needed to be.

> +/* Perform consistency checks on the current options.  */
> +
> +static void
> +mips_check_options (struct mips_set_options *opts, bfd_boolean abi_checks)
> +{
> +  /* Check the size of integer registers agrees with the ABI and ISA.  */
> +  if (opts->gp32 == 0 && !ISA_HAS_64BIT_REGS (opts->isa))
> +    as_bad (_("`gp64' used with a 32-bit processor"));
> +  else if (abi_checks == TRUE
> +	   && opts->gp32 == 1 && ABI_NEEDS_64BIT_REGS (mips_abi))
> +    as_bad (_("`gp32' used with a 64-bit ABI"));
> +  else if (abi_checks == TRUE
> +	   && opts->gp32 == 0 && ABI_NEEDS_32BIT_REGS (mips_abi))
> +    as_bad (_("`gp64' used with a 32-bit ABI"));
> +
> +  /* Check the size of the float registers agrees with the ABI and ISA.  */
> +  switch (opts->fp)
> +    {
> +    case 0:
> +      if (!CPU_HAS_LDC1_SDC1 (opts->arch))
> +	as_bad (_("`fpxx' used with a cpu lacking ldc1/sdc1 instructions"));
> +      else if (opts->single_float == 1
> +	       || opts->soft_float == 1)
> +	as_bad (_("`fpxx' cannot be used with `single-float' or `soft-float'"));
> +      break;
> +    case 64:
> +      if (!ISA_HAS_64BIT_FPRS (opts->isa))
> +	as_bad (_("`fp64' used with a 32-bit fpu"));
> +      else if (abi_checks == TRUE
> +	       && ABI_NEEDS_32BIT_REGS (mips_abi)
> +	       && !ISA_HAS_MXHC1 (opts->isa))
> +	as_warn (_("`fp64' used with a 32-bit ABI"));

You said before that -mfp64 and -msingle-float were compatible, but I'm not
sure they are really.  I think that should be an error too.

> +      if ((regno & 1) != 0)
> +	{
> +	  if (mips_opts.mips_defer_fp_warn == TRUE)
> +	    as_warn (_("Dangerous use of FP registers in fp%s when module is fp%s"),
> +		     (mips_opts.fp == 32) ? "32"
> +		     : (mips_opts.fp == 64) ? "64" : "xx",
> +		     (file_mips_opts.fp == 32) ? "32"
> +		     : (file_mips_opts.fp == 64) ? "64" : "xx");

I don't think we should warn about code that's compatible with the
current ".set fp".  How would you write a warning-free fp=64 ifunc?

> @@ -5322,13 +5527,12 @@ match_float_constant (struct mips_arg_info *arg, expressionS *imm,
>    /* Handle 64-bit constants for which an immediate value is best.  */
>    if (length == 8
>        && !mips_disable_float_construction
> -      /* Constants can only be constructed in GPRs and copied
> -	 to FPRs if the GPRs are at least as wide as the FPRs.
> -	 Force the constant into memory if we are using 64-bit FPRs
> -	 but the GPRs are only 32 bits wide.  */
> -      /* ??? No longer true with the addition of MTHC1, but this
> -	 is legacy code...  */
> -      && (using_gprs || !(HAVE_64BIT_FPRS && HAVE_32BIT_GPRS))
> +      /* Constants can only be constructed in GPRs and copied to FPRs if the
> +	 GPRs are at least as wide as the FPRs or MTHC1 is available.  */
> +      && (using_gprs
> +	  || HAVE_64BIT_GPRS
> +	  || ISA_HAS_MXHC1 (mips_opts.isa)
> +	  || (HAVE_32BIT_FPRS && mips_opts.fp != 0))

We should keep the bit about forcing it to memory otherwise.
Can you explain the HAVE_32BIT_FPRS && mips_opts.fp != 0 thing a bit more?
Maybe we should get rid of HAVE_32BIT_FPRS and HAVE_64BIT_FPRS now that
there are three alternatives and just test mips_opts.fp directly.
(Please do that as a separate patch though -- there are quite a few
different things going on in the current patch.)

> @@ -6690,7 +6894,8 @@ append_insn (struct mips_cl_insn *ip, expressionS *address_expr,
>  	     && (mips_opts.at || mips_pic == NO_PIC)
>  	     /* Don't relax BPOSGE32/64 or BC1ANY2T/F and BC1ANY4T/F
>  	        as they have no complementing branches.  */
> -	     && !(ip->insn_mo->ase & (ASE_MIPS3D | ASE_DSP64 | ASE_DSP)));
> +	     && !(ip->insn_mo->ase & (AFL_ASE_MIPS3D | AFL_ASE_DSP64
> +				      | AFL_ASE_DSP)));
>  
>    if (!HAVE_CODE_COMPRESSION
>        && address_expr

Would be great if you could split out the ASE renaming into a separate
patch too.

> @@ -13506,39 +13715,39 @@ md_parse_option (int c, char *arg)
>        break;
>  
>      case OPTION_MIPS1:
> -      file_mips_isa = ISA_MIPS1;
> +      file_mips_opts.isa = ISA_MIPS1;
>        break;
>  
>      case OPTION_MIPS2:
> -      file_mips_isa = ISA_MIPS2;
> +      file_mips_opts.isa = ISA_MIPS2;
>        break;
>  
>      case OPTION_MIPS3:
> -      file_mips_isa = ISA_MIPS3;
> +      file_mips_opts.isa = ISA_MIPS3;
>        break;
>  
>      case OPTION_MIPS4:
> -      file_mips_isa = ISA_MIPS4;
> +      file_mips_opts.isa = ISA_MIPS4;
>        break;
>  
>      case OPTION_MIPS5:
> -      file_mips_isa = ISA_MIPS5;
> +      file_mips_opts.isa = ISA_MIPS5;
>        break;
>  
>      case OPTION_MIPS32:
> -      file_mips_isa = ISA_MIPS32;
> +      file_mips_opts.isa = ISA_MIPS32;
>        break;
>  
>      case OPTION_MIPS32R2:
> -      file_mips_isa = ISA_MIPS32R2;
> +      file_mips_opts.isa = ISA_MIPS32R2;
>        break;
>  
>      case OPTION_MIPS64R2:
> -      file_mips_isa = ISA_MIPS64R2;
> +      file_mips_opts.isa = ISA_MIPS64R2;
>        break;
>  
>      case OPTION_MIPS64:
> -      file_mips_isa = ISA_MIPS64;
> +      file_mips_opts.isa = ISA_MIPS64;
>        break;
>  
>      case OPTION_MTUNE:

I think this consolidation into file_mips_opts should be a separate patch too
(without changing the logic).

> @@ -14922,30 +15091,11 @@ struct mips_option_stack
>  
>  static struct mips_option_stack *mips_opts_stack;
>  
> -/* Handle the .set pseudo-op.  */
> -
> -static void
> -s_mipsset (int x ATTRIBUTE_UNUSED)
> +static bfd_boolean
> +s_mipssettings (char * name)

s_foo is just for directives -- let's call this parse_code_option or
something (suggestions for better names welcome).

> @@ -15155,23 +15281,87 @@ s_mipsset (int x ATTRIBUTE_UNUSED)
>  	  free (s);
>  	}
>      }
> -  else if (strcmp (name, "sym32") == 0)
> -    mips_opts.sym32 = TRUE;
> -  else if (strcmp (name, "nosym32") == 0)
> -    mips_opts.sym32 = FALSE;
> -  else if (strchr (name, ','))
> +  else if (s_mipssettings (name) == FALSE)
> +    as_warn (_("tried to set unrecognized symbol: %s\n"), name);
> +
> +  /* The use of .set [arch|cpu]= historically 'fixes' the width of gp and fp
> +     registers based on what is supported by the arch/cpu.  */
> +  if (mips_opts.isa != prev_isa)
>      {
> -      /* Generic ".set" directive; use the generic handler.  */
> -      *input_line_pointer = ch;
> -      input_line_pointer = name;
> -      s_set (0);
> -      return;
> +      switch (mips_opts.isa)
> +	{
> +	case 0:
> +	  break;
> +	case ISA_MIPS1:
> +	case ISA_MIPS2:
> +	case ISA_MIPS32:
> +	case ISA_MIPS32R2:
> +	  mips_opts.gp32 = 1;
> +	  if (mips_opts.fp != 0)
> +	    mips_opts.fp = 32;
> +	  break;
> +	case ISA_MIPS3:
> +	case ISA_MIPS4:
> +	case ISA_MIPS5:
> +	case ISA_MIPS64:
> +	case ISA_MIPS64R2:
> +	  mips_opts.gp32 = 0;
> +	  if (mips_opts.arch == CPU_R5900)
> +	    {
> +	      if (mips_opts.fp != 0)
> +		mips_opts.fp = 32;
> +	    }
> +	  else if (mips_opts.fp != 0)
> +	    mips_opts.fp = 64;

Seems more natural as:

	  if (mips_opts.fp != 0)
	    {
	      if (mips_opts.arch == CPU_R5900)
		mips_opts.fp = 32;
	      else
		mips_opts.fp = 64;
	    }

> +/* Handle the .module pseudo-op.  */
> +
> +static void
> +s_module (int ignore ATTRIBUTE_UNUSED)
> +{
> +  char *name = input_line_pointer, ch;
> +  int prev_arch = file_mips_opts.arch;
> +
> +  while (!is_end_of_line[(unsigned char) *input_line_pointer])
> +    ++input_line_pointer;
> +  ch = *input_line_pointer;
> +  *input_line_pointer = '\0';
> +
> +  if (file_mips_opts_checked == FALSE)
>      {
> -      as_warn (_("tried to set unrecognized symbol: %s\n"), name);
> +      if (s_mipssettings (name) == FALSE)
> +	as_warn (_(".module used with unrecognized symbol: %s\n"), name);

!foo rather than foo == FALSE (throughout)

> +
> +      /* Update module level settings from mips_opts.  */
> +      file_mips_opts = mips_opts;
>      }
> -  mips_check_isa_supports_ases ();
> +  else
> +    as_warn (_("ignoring .module after generating code"));

I think this should be an as_bad.

> +  if (prev_arch != file_mips_opts.arch
> +      && ! bfd_set_arch_mach (stdoutput, bfd_arch_mips, file_mips_opts.arch))
> +    as_warn (_("could not set architecture and machine"));

I think we should do this unconditionally when checking the options
rather than here.

> @@ -18272,3 +18560,33 @@ tc_mips_regname_to_dw2regnum (char *regname)
>  
>    return regnum;
>  }
> +
> +/* Given a symbolic attribute NAME, return the proper integer value.
> +   Returns -1 if the attribute is not known.  */
> +
> +int
> +mips_convert_symbolic_attribute (const char *name)
> +{
> +  static const struct
> +  {
> +    const char * name;
> +    const int    tag;
> +  }
> +  attribute_table[] =
> +    {
> +#define T(tag) {#tag, tag}
> +      T (Tag_GNU_MIPS_ABI_FP),
> +      T (Tag_GNU_MIPS_ABI_MSA),
> +#undef T
> +    };
> +  unsigned int i;
> +
> +  if (name == NULL)
> +    return -1;
> +
> +  for (i = 0; i < ARRAY_SIZE (attribute_table); i++)
> +    if (streq (name, attribute_table[i].name))
> +      return attribute_table[i].tag;
> +
> +  return -1;
> +}

I think this should be a separate patch too.

> diff --git a/gas/doc/c-mips.texi b/gas/doc/c-mips.texi
> index 0c5e82d..37926d4 100644
> --- a/gas/doc/c-mips.texi
> +++ b/gas/doc/c-mips.texi
> @@ -28,6 +28,7 @@ Assembly Language Programming'' in the same work.
>  * MIPS assembly options:: Directives to control code generation
>  * MIPS autoextend::	Directives for extending MIPS 16 bit instructions
>  * MIPS insn::		Directive to mark data as an instruction
> +* MIPS FP ABIs::	Marking which FP ABI is in use
>  * MIPS NaN Encodings::	Directives to record which NaN encoding is being used
>  * MIPS Option Stack::	Directives to save and restore options
>  * MIPS ASE Instruction Generation Overrides:: Directives to control
> @@ -119,6 +120,15 @@ The @code{.set gp=64} and @code{.set fp=64} directives allow the size
>  of registers to be changed for parts of an object. The default value is
>  restored by @code{.set gp=default} and @code{.set fp=default}.
>  
> +@item -mfpxx
> +Make no assumptions about whether 32-bit or 64-bit registers are available.

...floating-point registers...?

> +This is provided to support having modules compatible with either
> +@samp{-mfp32} or @samp{-mfp64}. This option can only be used with MIPS II
> +and above.
> +
> +The @code{.set fp=xx} directive allows a part of an object to be marked
> +as not making assumptions about 32-bit or 64-bita FP registers.  The
> +default value is restored by @code{.set fp=default}.
>  @item -mips16
>  @itemx -no-mips16
>  Generate code for the MIPS 16 processor.  This is equivalent to putting
> @@ -687,6 +697,22 @@ Traditional MIPS assemblers do not support this directive.
>  @node MIPS assembly options
>  @section Directives to control code generation
>  
> +@cindex MIPS directives to override command line options
> +@kindex @code{.module}
> +The @code{.module} directive allows command line options to be set directly
> +from assembly.  The format of the directive matches the @code{.set}
> +directive but only those options which are relevant to a whole module are
> +supported.  The effect of a @code{.module} directive is the same as the
> +corresponding command line option.  Where @code{.set} directives support
> +returning to a default then the @code{.module} directives do not as they
> +define the defaults.
> +
> +These module level directives must appear first in assembly and will raise

Nit: module-level

> +a warning if found after the first instruction, @code{.set} directive or
> +any code generating directive.

If it becomes a hard error we can remove the "and will raise..." bit.

> @@ -749,6 +775,108 @@ baz:
>  
>  @end example
>  
> +@node MIPS FP ABIs
> +@section Directives to control the FP ABI
> +@menu
> +* MIPS FP ABI History::                History of FP ABIs
> +* MIPS FP ABI Variants::               Supported FP ABIs
> +* MIPS FP ABI Selection::              Automatic selection of FP ABI
> +* MIPS FP ABI Compatibility::          Linking different FP ABI variants
> +@end menu
> +
> +@node MIPS FP ABI History
> +@subsection History of FP ABIs
> +@cindex @code{.gnu_attribute 4, @var{n}} directive, MIPS
> +@cindex @code{.gnu_attribute Tag_GNU_MIPS_ABI_FP, @var{n}} directive, MIPS
> +The MIPS ABIs support a variety of different floating-point extensions
> +where calling-convention and register sizes vary for floating-point data.
> +The extensions exist to support a wide variety of optional architecture
> +features.  The resulting ABI variants are generally incompatible with each
> +other and must be tracked carefully.
> +
> +Traditionally the use of an explicit @code{.gnu_attribute 4, @var{n}}
> +directive is used to indicate which ABI is in use by a specific module.
> +It was then left to the user to ensure that command line options and the
> +selected ABI were compatible with some potential for inconsistencies.
> +
> +@node MIPS FP ABI Variants
> +@subsection Supported FP ABIs
> +The supported floating-point ABI variants are:
> +
> +@table @code
> +@item 0 - No floating-point
> +This variant is used to indicate that floating-point is not used within
> +the module at all and therefore has no impact on the ABI.  This is the
> +default.
> +
> +@item 1 - Double-precision
> +This variant indicates that double-precision support is used.  For 64-bit
> +ABIs this means that 64-bit wide floating-point registers are required.
> +For 32-bit ABIs this means that 32-bit wide floating-point registers are
> +required and double precision operations across pairs of registers.

found the last sentence hard to parse: s/across/use/, or something?

> +@item 2 - Single-precision
> +This variant indicates that single-precision support is used.  This is
> +generally taken to mean that the ABI is also modified such that
> +sizeof (double) == sizeof (float).  This has an impact on calling
> +convention and callee-save behaviour.

I'm not sure about the sizeof (double) thing: I think it's legitimate
to use -msingle-float with a 32-bit FPU and use software libraries
for double-precision.

> +@node MIPS FP ABI Selection
> +@subsection Automatic selection of FP ABI
> +@cindex @code{.module fp=@var{nn}} directive, MIPS
> +In order to simplify and add safety to the process of selecting the
> +correct floating-point ABI, the assembler will automatically infer the
> +correct @code{.gnu_attribute 4, @var{n}} directive based on command line
> +options @code{.module} overrides and instruction usage.  Where an explicit
> +@code{.gnu_attribute 4, @var{n}} directive has been seen then a warning
> +will be raised if it does not match an inferred setting.

Obviously the "and instruction usage" is the contentious bit here :-)

I think it'd be better to change gp32 into gp too, for consistency.
I can do that as a follow-up though.

Thanks,
Richard