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

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

> -----Original Message-----
> From: Richard Sandiford [mailto:rdsandiford@googlemail.com]
> Sent: 07 May 2014 14:38
> To: Matthew Fortune
> Cc: macro@codesourcery.com; Joseph Myers (joseph@codesourcery.com);
> binutils@sourceware.org; Rich Fuhler
> Subject: Re: [PATCH 1/2] Add support for O32 FPXX ABI
> 
> 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.

Agreed. Ignore previous comments. I'll remove the warning.

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

True, and have that extra flag take part in the compatibility checks.

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

I'll drop this topic for now but I expect to have to reopen 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.
> 
> Code explicitly marked .gnu_attribute 4,0 should end up as FP_ANY.

So I should move the setting of the gnu_attribute based on command-line
module options into the file level option checking code so that it can
be overridden by the user later. Otherwise, I won't be able to tell the
difference between the default (4,0) and an explicit (4,0).

Regards,
Matthew