RE: [PATCH][MIPS] Implement O32 FPXX ABI (GCC)

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

> Let's step back a bit.  Please explain which case you were trying to
> handle with the specs patch.  Rejecting -msingle-float -mfp64 seems
> fine.
> Fiddling with the specs to stop that combination reaching the assembler
> is what seemed odd.

So, perhaps this is a 'vendor' issue too like some other debates we have
had in this area. I presume the --with arguments are intended to
be used to configure a single purpose toolchain that targets a specific
ABI. So if used to construct a single-float toolchain then --with-fp=64
would not be used. And if configured using --with-fp=64 then it is OK to
have an error if using -msingle-float.

I am actually considering implications of adding support for MIPS64r6
to GCC which I have not posted yet, sorry :-). MIPSr6 only supports
64-bit registers which naturally leads to needing -mfp64. MIPSr6 does
however also support a single-precision only variant.

For a single purpose native toolchain then --with-fp=64 can be used xor
--with-fpu=single to get the desired tools.

In a multilib toolchain then the vendor specific DRIVER_SELF_SPECS need
to infer -mfp64 for -mabi=32 and MIPSr6.  The FPXX patch currently has:

mti-linux.h:
  /* If no FP option is specified, infer one from the ABI/ISA level.  */\
  "%{!mfp*: %{mabi=32: %{" MIPS_FP32_OPTION_SPEC                        \
  ": -mfp32;: -mfpxx}}}",                                               \
 
With MIPSr6 added this would naturally become

mti-linux.h:
  /* If no FP option is specified, infer one from the ABI/ISA level.  */\
  "%{!mfp*: %{mabi=32: %{" MIPS_FP32_OPTION_SPEC                        \
  ": -mfp32;%{" MIPS_FP64_OPTION_SPEC ": -mfp64;: -mfpxx}}}",

With:
#define MIPS_FP64_OPTION_SPEC "mips32r6"

But I suppose it could be changed to the following such that
msingle-float prevents inferring a non-default -mfp setting.

mti-linux.h:
  /* If no FP option is specified, infer one from the ABI/ISA level.  */\
  "%{!mfp*: %{mabi=32: %{!msingle-float:%{" MIPS_FP32_OPTION_SPEC                        \
  ": -mfp32;" MIPS_FP64_OPTION_SPEC ": -mfp64;: -mfpxx}}}}",

With:
#define MIPS_FP64_OPTION_SPEC "mips32r6"

The goal is to support a multilib toolchain which will generate correct
code by just using the architecture option and then be able to add other
options for non-default ABI variations.

mips-mti-linux-gnu -mips32r6 ==> gets O32 FP64
mips-mti-linux-gnu -mips32r6 -msingle-float ==> gets O32 FPSINGLE
mips-mti-linux-gnu -mips32r5 ==> gets O32 FPXX
mips-mti-linux-gnu -mips1 ==> gets O32 FP32

It seems strange if the -mips32r6 -msingle-float required you to set
-mfp32 as that should not matter. Single-float can only possibly mean
32-bit registers.

> >> >> > diff --git a/gcc/testsuite/gcc.target/mips/call-clobbered-2.c
> >> >> b/gcc/testsuite/gcc.target/mips/call-clobbered-2.c
> >> >> > new file mode 100644
> >> >> > index 0000000..f47cdda
> >> >> > --- /dev/null
> >> >> > +++ b/gcc/testsuite/gcc.target/mips/call-clobbered-2.c
> >> >> > @@ -0,0 +1,21 @@
> >> >> > +/* Check that we handle call-clobbered FPRs correctly.  */
> >> >> > +/* { dg-skip-if "code quality test" { *-*-* } { "-O0" } { "" }
> } */
> >> >> > +/* { dg-options "-mabi=32 -modd-spreg -mfp32 -ffixed-f0 -
> ffixed-f1 -
> >> >> ffixed-f2 -ffixed-f3 -ffixed-f4 -ffixed-f5 -ffixed-f6 -ffixed-f7 -
> >> ffixed-f8
> >> >> -ffixed-f9 -ffixed-f10 -ffixed-f11 -ffixed-f12 -ffixed-f13 -
> ffixed-f14 -
> >> >> ffixed-f15 -ffixed-f16 -ffixed-f17 -ffixed-f18 -ffixed-f19 -
> ffixed-f20 -
> >> >> ffixed-f22 -ffixed-f24 -ffixed-f26 -ffixed-f28 -ffixed-f30" } */
> >> >> > +
> >> >> > +void bar (void);
> >> >> > +float a;
> >> >> > +float
> >> >> > +foo ()
> >> >> > +{
> >> >> > +  float b = a + 1.0f;
> >> >> > +  bar();
> >> >> > +  return b;
> >> >> > +}
> >> >> > +/* { dg-final { scan-assembler-times "lwc1" 2 } } */
> >> >> > +/* { dg-final { scan-assembler-not "swc1" } } */
> >> >> > +/* { dg-final { scan-assembler-times "sdc1" 2 } } */
> >> >> > +/* { dg-final { scan-assembler-not "mtc" } } */
> >> >> > +/* { dg-final { scan-assembler-not "mfc" } } */
> >> >> > +/* { dg-final { scan-assembler-not "mthc" } } */
> >> >> > +/* { dg-final { scan-assembler-not "mfhc" } } */
> >> >>
> >> >> Why require sdc1 here?  Would Chao-Ying's patch make this use SWC1
> and
> >> LWC1
> >> >> exclusively?
> >> >
> >> > The SDC1 instructions are for callee-saved registers. I'll add the
> >> > check for two corresponding LDC1 instructions in the epilogue
> though
> >> > since I've noticed them being missing.
> >>
> >> I'm probably being dense, sorry, but why is SDC1 needed for -mfp32
> >> -modd-spreg?  Can't we just save and restore the odd register?
> >> That's technically more correct too, since we shouldn't really touch
> >> -ffixed registers.
> >
> > You are correct and I am/was aware of this...
> >
> > GCC is saving too much of the callee-saved registers when single-
> precision
> > values are live in them but this is historic behaviour. The code which
> > controls this is very complex and I'd be worried about breaking it.
> The
> > fix would be invasive as all the logic is honed towards the notion of
> > 64-bit callee-saved registers.
> 
> But that's because it was written before separate odd spregs came along.
> I'm not convinced the situation is as bad as you say.
>
> > One thing to say is that this test is
> > a very aggressive test of ABI not normal compiler behaviour. Normally
> > an even-numbered callee-saved register would be used first followed by
> the
> > odd meaning that it is rare to only use the odd register. That flips
> the
> > problem round to single precision data in even registers...
> >
> > I believe that prior to mips32 introducing odd-numbered single-
> precision
> > registers then GCC will have been saving and restoring using sdc1/ldc1
> > for even-numbered registers regardless of whether they are used for
> > single or double precision data (for mips1 it also will do pairs of
> > lwc1/swc1).
> 
> Sure.  And saving and restoring both using LDC1 and SDC1 is still
> the right thing to do if both registers are clobbered.  But if only
> one is clobbered then we should save just that.  It's really just
> the same principle as Chao-ying's patch, but applied to the prologue
> and epilogue.
> 
> > My vote FWIW is to leave this behaviour as is, if you would like it
> looking
> > in to then it is a separate issue to FPXX I believe. Going forward
> with
> > the FPXX ABI becoming the norm then we have to save/restore 64-bit
> registers
> > anyway so the fix may not be of much value for end users.
> 
> It isn't really that separate from -modd-spreg.  We shouldn't add tests
> for wrong output.

I'm not confident that it would be the right thing to change this
behaviour. While the test is slightly weird in that the compiler generates
code that touches a -ffixed-reg this is just an ABI test.

== Taken from MIPS ABI supplement ==
Only registers $f20.$f30 are preserved across a function call. All other float-
ing-point registers can change across a function call. However, functions
that use any of $f20.$f30 for single-precision operations only must still save
and restore the corresponding odd-numbered register since the odd-num-
bered register contents are left undefined by single-precision operations
== end == 

There are comments in the code to assert that this is behaviour is very much
intentional as well:

== mips.c ==
static bool
mips_save_reg_p (unsigned int regno)
{
  if (mips_cfun_call_saved_reg_p (regno))
    {
      if (mips_cfun_might_clobber_call_saved_reg_p (regno))
        return true;

      /* Save both registers in an FPR pair if either one is used.  This is
         needed for the case when MIN_FPRS_PER_FMT == 1, which allows the odd
         register to be used without the even register.  */
      if (FP_REG_P (regno)
          && MAX_FPRS_PER_FMT == 2
          && mips_cfun_might_clobber_call_saved_reg_p (regno + 1))
        return true;
    }

  /* We need to save the incoming return address if __builtin_eh_return
     is being used to set a different return address.  */
  if (regno == RETURN_ADDR_REGNUM && crtl->calls_eh_return)
    return true;

  return false;
}
== end ==

I therefore take back my comment about the prologue/epilogue saving too much
callee-save register state :-) It is doing exactly what is required by the
arch. I doubt very much that any MIPS implementation would end up with
incorrect operation if you ran the following and ended up with $f18 and $f20
being different or undefined behaviour if $f20 were accessed as a double but 
that is actually what the spec says.

ADD.D $f20, $f10, $f10
MOV.D $f18, $f20
SWC1 $f20, 0($sp)
MTC1 $2, $f20
LWC1 $f20, 0($sp)
ADD.D $f16, $f18, $f20
($f16 should be 4*$f10)

Presumably, to account for MIPS I then a pair of SWC1s/LWC1s are required
by the architecture to allow the resulting even numbered register to be
accessed as a double.

MIPS32 to my knowledge makes no re-assuring statement that accessing the
odd numbered register with a single precision operation does not clobber
the lower 32-bits of the double it is overlaid with.

I think all of that means GCC is correct.

Regards,
Matthew