Re: [PATCH] Better checking of ISA/ASE/ABI options for MIPS gas

[Richard Sandiford <richard at codesourcery dot com>]

FWIW, as a bystander, I like this.  Just a couple of things:

> I'm somewhat uncertain about the ABI incompatibility warning for
> wrong FP register widths, does it make sense to force a different
> FP register width in the assembler in some cases?

I agree with you and Eric that complaining rather than overriding
is the right way to go.

Thiemo Seufer <ths@networkno.de> writes:
> +#define ISA_SUPPORT_DSP_ASE	(mips_opts.isa == ISA_MIPS32R2       \
> +				 || mips_opts.isa == ISA_MIPS64R2)
> +
>  /* True if -mmt was passed or implied by arguments passed on the
>     command line (e.g., by -march).  */
>  static int file_ase_mt;
>  
> +#define ISA_SUPPORT_MT_ASE	(mips_opts.isa == ISA_MIPS32R2       \
> +				 || mips_opts.isa == ISA_MIPS64R2)
> +

Nitpick: ISA_SUPPORTS rather than ISA_SUPPORT.  (I see ISA_SUPPORT_SMARTMIPS
is already in, but SUPPORTS is grammatically correct, and more consistent
with other macros like ISA_HAS.)

> +/*  Return true if ISA supports 64 bit float register instructions.  */
> +#define ISA_HAS_64BIT_FPRS(ISA)		\
> +  ((ISA) == ISA_MIPS3			\
> +   || (ISA) == ISA_MIPS4		\
> +   || (ISA) == ISA_MIPS5		\
> +   || (ISA) == ISA_MIPS32R2		\
> +   || (ISA) == ISA_MIPS64		\
> +   || (ISA) == ISA_MIPS64R2)
> +

Another nitpick: "64 bit float register instructions" seems a bit woolly.
In the subset of instructions supported by ISA_MIPS3, I don't think any
instructions are inherently "32 bit float register instructions" or
"64 bit float register instructions".  It's a property of the processor
mode rather than the instruction itself.  I realise that, as far as the
ISA_MIPS3 subset goes, you probably mean "instructions with odd-numbered
register operands", but the comment doesn't make that immediately clear.
The macro name seems more accurate than the comment.

> -  /* ??? GAS treats single-float processors as though they had 64-bit
> -     float registers (although it complains when double-precision
> -     instructions are used).  As things stand, saying they have 32-bit
> -     registers would lead to spurious "register must be even" messages.
> -     So here we assume float registers are always the same size as
> -     integer ones, unless the user says otherwise.  */
> -  if (file_mips_fp32 < 0)
> -    file_mips_fp32 = file_mips_gp32;
> +  switch (file_mips_fp32)
> +    {
> +    default:
> +    case -1:
> +      /* No user specified float register size.  */
> +      if (file_mips_gp32 == 0)
> +	/* 64-bit integer registers implies 64-bit float registers.  */
> +	file_mips_fp32 = 0;
> +      else if ((mips_opts.ase_mips3d > 0 || mips_opts.ase_mdmx > 0)
> +	       && ISA_HAS_64BIT_FPRS (mips_opts.isa))
> +	/* -mips3d and -mdmx imply 64-bit float registers, if possible.  */
> +	file_mips_fp32 = 0;
> +      else
> +	/* 32-bit float registers. */
> +	file_mips_fp32 = 1;
> +      break;
> +
> +    /* The user specified the size of the float registers.  Check if it
> +       agrees with the ABI and ISA.  */
> +    case 0:
> +      if (!ISA_HAS_64BIT_FPRS (mips_opts.isa))
> +	as_bad (_("-mfp64 used with a 32-bit fpu"));
> +      else if (ABI_NEEDS_32BIT_REGS (mips_abi)
> +	       && !ISA_HAS_MXHC1 (mips_opts.isa))
> +	as_warn (_("-mfp64 used with a 32-bit ABI"));
> +      break;
> +    case 1:
> +      if (ABI_NEEDS_64BIT_REGS (mips_abi))
> +	as_warn (_("-mfp32 used with a 64-bit ABI"));
> +      break;
> +    }

Doesn't the ??? comment still hold?  I thought it would be valuable
to keep it.

> +  else if (strcmp (name, "gp64") == 0)
> +    {
> +      if (!ISA_HAS_64BIT_REGS (mips_opts.isa))
> +	as_warn ("%s isa does not support 64-bit registers", 
> +		 mips_cpu_info_from_isa (mips_opts.isa)->name);
> +      mips_opts.gp32 = 0;
> +    }
> +  else if (strcmp (name, "gp32") == 0)
> +    {
> +      mips_opts.gp32 = 1;
> +    }
> +  else if (strcmp (name, "nogp64") == 0 || strcmp (name, "nogp32") == 0)
> +    {
> +      mips_opts.gp32 = file_mips_gp32;
> +    }

Ugh.  I don't like the "nogp32" and "nogp64".  All other ".set X"/".set noX"
pairs are used for turning a particular feature on and off.  Having "nogp32"
restore the prevailing size (even if that prevailing size _is_ gp32)
seems very counter-intuitive to me.  Could we not have ".set gp=" instead,
with a special value to select the prevailing size?  That would be more
consistent with .set mipsX and .set arch=X, for example.

Richard