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

[Richard Sandiford <rdsandiford at googlemail dot com>]

Matthew Fortune <Matthew.Fortune@imgtec.com> writes:
>> >> > +      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 not though?  What if you build an application that uses one FR setting
but has some interface code to allow it run plugins from the other?
That interface code would need to be written in asm and could legitimately
change the fp setting.

.set is there for that kind of thing, or other such local violations
of the ABI for reasons that tool writers can't predict in advance.

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

It was an "if we're going to" in the sense of "I don't think we should". :-)

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

What I meant by the FPR_SIZE thing was something like:

#define FPR_SIZE \
  (mips_opts.fp == 64 && !ISA_HAS_64BIT_FPRS (mips_opts.isa) \
   ? 32 \
   : mips_opts.fp)

Then replace all HAVE_32BIT_FPRS checks with FPR_SIZE checks.
This particular case would use FPR_SIZE == 32.

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

This attribute is about the FPU rather than C types though.  I think any
-fshort-double annotation should be a separate flag.  There's nothing to
stop you using it with -msoft-float too.

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

No, the GCC driver should still direct the assembler and linker as well
as the compiler proper.

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

Code explicitly marked .gnu_attribute 4,0 should end up as FP_ANY.

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

Obviously it can't be perfect.  But what I meant was: you had code to
infer the ABI from the instructions.  That implies that some instructions
rule out some of the -mhard-float possibilities.  Rather than infer the ABI
from that information, we should turn it around and diagnose cases where
an instruction is inconsistent with the current mips_opts.  E.g. using
"add.d $f1, $f1, $f1" in fp=xx code would get a diagnostic.

Thanks,
Richard