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

[Matthew Fortune <Matthew dot Fortune at imgtec dot com>]

Richard Sandiford <rdsandiford@googlemail.com> writes:
> 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.

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

OK

<snip>

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

The check is there because the code simply has no chance at all of getting
the correct mode at runtime if the module mode is not FPXX.

> 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 you strictly stick to accessing even numbered registers then the code
will run correctly on FR=0 or FR=1 and the subset of the three calling
conventions you can then use are all identical. However...

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

I'm fine with doing this instead, there is little value in allowing a
.set fp=[32|64] when the region of code it encompasses does not really
use any floating-point or uses a subset of floating-point.

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

I'd change the definition of HAVE_32BIT_FPRS if I was sure enough that it
wouldn't break things but I can't tell.

Some of this ugliness comes out of the fact that we have three sources
of information about FP state: singlefloat, softfloat and fp. Merging
these three together to give a single floating-point status may be a good
idea but I don't really want to do that in this patch.

Am I OK to just change this:
+	  || (HAVE_32BIT_FPRS && mips_opts.fp != 0))
To either:
+	  || (HAVE_32BIT_FPRS && mips_opts.fp == 32))
Or more boldly
+	  || (mips_opts.fp == 32))


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

Indeed it will only happen once anyway.

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

OK. But we will need to introduce a further FP ABI for short-double as part
of R6 (bare metal only) then. I haven't looked to see what the hybrid
calling convention ends up as for single-float (without short-double) I
wonder what happens to doubles.
 
> >> > +@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.

True but GCC will be passing these flags implicitly. Perhaps I'm attacking
this issue from the wrong side. Would you be OK with us no longer passing:

-msoft-float, -mfpxx, -mfp64, -msingle-float

from compiler driver to assembler and instead rely on the fact that the
compiler will have emitted a .module directive to indicate which of these
is active. For hand-crafted assembly code assembled via the GCC driver
it will get default -mfp32/double-float. 'Correct' multi-abi hand written
assembly code should be pre-processed and use the __mips_fpr and
__mips_soft_float etc macros to set a .module directive and correctly
enable the code for the selected ABI (or error if the module doesn't
support it).

> > 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 was trying to point out that if I were to remove the code that checks
if any FP code has been seen before inferring a hard-float ABI, then
there would be no way for a mode to end up marked as ABI_FP_ANY. The
patch as written will support 4,0 iff no floating point instruction has
been included in the text and not targeting soft-float.

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

I can't think of any assembler level checks that can determine whether
FP code complies with the ABIs or not. 

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

OK. Let's look at GCC specs as an alternative way to get some safety in
all this. I'll post the GCC patch today.

Regards,
Matthew