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

[Richard Sandiford]

Matthew Fortune <Matthew.Fortune@imgtec.com> writes:
>> > +  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.
>
> Are you saying we should ignore the isa_ext inputs and just use the new
> overall arch to set this?

Maybe bfd_set_arch_mach isn't the best place, but the reason I suggested it
was that the bfd_mach of the inputs should already reflect the architecture
in the input abiflags.  That might mean adding some more bfd_machs if there
isn't one for every AFL_EXT_*.

The bfd_mach is also used to perform the compatibility check
(mips_mach_extends_p in the code quoted above).  If the input bfd_mach isn't
correct at that point then we're not going to do the check properly.

> The potential problem with bfd_set_arch_mach is that it is called in a
> variety of different places and it can't set just the isa_ext field
> and then mark the abiflags as valid. I'm probably being dumb but I'm
> not sure how/where to hook this function either? I haven't quite got
> my head around BFD target vectors.

OK, I hadn't thought about the partial initialisation problem.
In that case I agree it might be better to keep it here.  But setting
the output isa_ext (and isa_level and isa_rev?) should at least be done 
when calling bfd_set_arch_mach in the code above.  Please put the code
in a helper function though.

>> > +/* 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.
>
> OK. I expect you are only referring to large comments not the inline comments
> For field names.

Yeah, sorry, it was the duplication of the "Note ..." part.

>> > +/* 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.
>
> This area is fuzzy at best. I've seen evidence both ways round in binutils
> (I can't point it out right now though) but GCC certainly only supports
> -mfp32 -msingle-float so I'd be happy with enforcing that in gas and
> dealing with any potential issues.

Thanks.

>> > +      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?
>
> This is perhaps a non-intuitive piece of code but the mips_defer_fp_warn flag
> will only be set if the overall module was not FPXX, the current mode is
> not FPXX and the two do not match. The only way to support having both FP32
> and FP64 in the same module is if the overall module is FPXX. The
> warning handles
> the case of 'xx' when in reality it can never happen. It can only print
> fp32,fp64
> or fp64,fp32. The reason the overall module must be FPXX (or matching
> the current
> mode) is because that is the only way you get the choice of mode at
> runtime (FPXX)
> or the correct mode to start with (FP64).

So the warning is about cases like ".module fp=32 .... .set fp=64 .... "
and ".module fp=64 .... .set fp=32 .... "?  I still think that if the user
has code in a .set fp=NN block, we should assume that they really do want
to write some code for NN-bit FPRs.

Why does the oddness of the register matter in these cases?  I can see
why it would matter for xx because of the special call-save/call-clobber
rules, but why is it as special when the FPR sizes of the module and the
.set block are known?  E.g. using an even FPR in fp=32 code is going to
clobber more registers than it would in fp=64 mode.

If we're going to treat this as a problem, I think we should do it when
the ".set" is first encountered.

>> > @@ -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.)
>
> The final clause is to allow 64-bit constant generation when the value would
> need to be moved to FPRs using a pair of MTC1s (not MTHC1). I retained the
> HAVE_32BIT_FPRS clause as it expands to more than just fp != 64, but I don't
> understand why it is more complex than that. The fp != 0 could just as easily
> have been fp == 32 which is probably clearer and is there to ensure that
> we don't break the FPXX ABI by using an MTC1 for the high part of a 64-bit
> value.
>
> There are still only two widths of register. The fact we are targeting FPXX
> is only relevant in a small number of places, otherwise it is simply 32-bit
> registers. So I would be changing HAVE_32BIT_FPRS to fp != 64 anyway.

That's fine.  I still think it's clearer, since it's fairly obvious that
fp != 64 means "not fp=64 mode", whereas it isn't as obvious that
HAVE_32BIT_FPRS means "fp=32 or fp=xx" mode.  Having to combine checks
for HAVE_NNBIT_FPRS and mips_opts.fp seems a bit weird.

I'd missed that HAVE_32BIT_FPRS depends on the ISA.  I'm not sure that's
a good idea either, but if we have to keep it, maybe a macro like FPR_SIZE
would be better than direct checks for mips_opts.fp.

>> > +  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.
>
> OK. I was cautious as I didn't know if there would be any side effects
> of calling bfd_set_arch_mach unnecessarily. 

Sorry, when I said "checking the options" I meant "checking the
file-level options".  That should happen exactly once per assembly and
seems the right time to set the bfd_mach on the result.

>> > +@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.
>
> Granted. But I believe the intent of the existing single-precision ABI is
> that it is also -fshort-double in GCC terms (r5900 I believe). We would
> need to track both single-float and single-float short-double separately.
> There are no current plans (from us at least) to support just
> single-precision with softfloat doubles.  The R6 spec re-introduces the
> idea of a single precision only FPU and this is planned to be primarily
> supported with short-doubles.

But emulated double-precision is definitely supported (and IIRC was also
used for r5900 GNU/Linux).  Maybe:

   Any double-precision operations must be emulated by software.

instead?

>> > +@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 :-)
>
> If we go with not having any safety net for hand written assembler then 
> instruction usage does still need to be included in this decision in order
> to support getting modules that end up as FP 'ANY' i.e. no-float. Otherwise
> we must write-off the ability to generate modules without a specific FP ABI
> because there is no way to detect if a user has written .gnu_attribute 4,0
> nor is there a command-line/module option to assert it.

If no attributes or command-line options are given, we can use the default
command-line setting for the configuration, which I think at the moment
is always -mdouble-float.  This will give a specific ABI.

This is just like the way that a file assembled without any other
information will get the default ISA level and CPU.

> Any thoughts on what to do with attribute 4,0? Since it is not very safe
> in GCC terms then we could just not support it I suppose.

We need a way for code that doesn't use FP to say that it doesn't care
about the FPU setting, so 4,0 seems fine to me.

> I expect you can guess that I'm still not convinced by providing no safety net
> at all for hand-written code :-) But I do understand the desire to keep things
> clear cut. The original plan would have avoided this issue as it defined new
> directives to get into fpxx. Perhaps it won't be an issue but I really wanted
> to be able to provide distribution maintainers with a tool to detect which
> objects remained FP32 even after the transition to an FPXX toolchain.

To be clear, I'm all in favour of warning about code that is wrong according
to the current mips_opts.  My objections were about not trusting ".set"
and about having complicated attempts to infer the ABI.

If the code is modern enough to use .gnu_attribute, we'll warn or error
(maybe the latter is best) about it being incompatible with -mfpxx.
If the code isn't modern enough to use .gnu_attribute and doesn't use
.set fp=, we should warn about anything that is obviously incompatible
with fp=xx (although clearly the warning can't be perfect).  In those
two cases we have the safety net.

If the code doesn't use .gnu_attribute but does use .set fp=, we simply
can't tell it apart from code is validly assuming a particular FPR size
(after appropriate checking).

Thanks,
Richard