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

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

Richard Sandiford <rdsandiford@googlemail.com> writes:
> Matthew Fortune <Matthew.Fortune@imgtec.com> writes:
> >> > *) Because GCC can be built to have mfpxx or mfp64 as the default
> option
> >> >    the ASM_SPEC has to handle these specially such that they are not
> >> >    passed in conjunction with -msingle-float. Depending on how all
> this
> >> >    option handling pans out then this may also need to account for
> >> >    msoft-float as well. It is an error to have -msoft-float and -mfp64
> in
> >> >    the assembler.
> >>
> >> The assembler and GCC shouldn't treat the options differently though.
> >> Either it should be OK for both or neither.
> >
> > I wasn't sure if you were applying this rule to options set by --with- at
> > configure time as well as user supplied options. If I move this logic
> > to OPTION_DEFAULT_SPECS instead and not apply the --with-fp option if
> > -msingle-float is given then that will fix the issue too. Is that OK?
> 
> --with-foo is just a way of setting the default -mfoo option when no
> explicit -mfoo option is given.  We shouldn't generally distinguish
> between them.

OK. I don't mind what we do here so I'm happy to just relax the (new)
assembler restrictions that would prevent -msingle-float -mfp64. I need to
check if these ended up in the .module binutils patch or are in the FPXX
patch.
 
> >> > @@ -5141,7 +5141,7 @@ mips_get_arg_info (struct mips_arg_info *info,
> >> const CUMULATIVE_ARGS *cum,
> >> >  			 || SCALAR_FLOAT_TYPE_P (type)
> >> >  			 || VECTOR_FLOAT_TYPE_P (type))
> >> >  		     && (GET_MODE_CLASS (mode) == MODE_FLOAT
> >> > -			 || mode == V2SFmode)
> >> > +			 || (TARGET_PAIRED_SINGLE_FLOAT && mode == V2SFmode))
> >> >  		     && GET_MODE_SIZE (mode) <= UNITS_PER_FPVALUE);
> >> >        break;
> >>
> >> This looks odd.  We shouldn't have V2SF values if there's no ISA support
> >> for them.
> >
> > True. This is a safety measure against future vector support. I/We wish
> to
> > completely restrict this ABI extension to the paired single-float
> > hardware feature. This detail is easy to forget, I would like to keep
> > the check but you get final call of course.
> 
> But the problem is that the change gives a specific behaviour for
> !TARGET_PAIRED_SINGLE_FLOAT && mode == V2SFmode, which at the moment
> should be a nonsensical condition.  I want to avoid defining an ABI
> for something that shouldn't happen.
> 
> I'd be happy with an assert like:
> 
>   gcc_assert (TARGET_PAIRED_SINGLE_FLOAT || mode != V2SFmode);

OK. It's just for future safety so this looks good.

> instead.
> >> > @@ -12202,7 +12247,8 @@ mips_secondary_reload_class (enum reg_class
> >> rclass,
> >> >  	return NO_REGS;
> >> >
> >> >        /* Otherwise, we need to reload through an integer register.
> */
> >> > -      return GR_REGS;
> >> > +      if (regno >= 0)
> >> > +        return GR_REGS;
> >> >      }
> >> >    if (FP_REG_P (regno))
> >> >      return reg_class_subset_p (rclass, GR_REGS) ? NO_REGS : GR_REGS;
> >>
> >> Why's this change needed?  Although I assume it's dead code if you
> tested
> >> against LRA.
> >
> > It is an optimisation actually (and this code is used by LRA). Without it
> > We end up with reload registers for floating point values being reloaded
> > via GR_REGS even though they can be stored to memory directly if a
> > FP_REG was used. This results in extra moves between GP and FP. The test
> > cases will fail if this code is removed (call-clobbered-3 and
> > call-clobbered-5).
> 
> Ah, OK.  In that case I think we should instead change:
> 
>       if (MEM_P (x)
> 	  && (GET_MODE_SIZE (mode) == 4 || GET_MODE_SIZE (mode) == 8))
> 	/* In this case we can use lwc1, swc1, ldc1 or sdc1.  We'll use
> 	   pairs of lwc1s and swc1s if ldc1 and sdc1 are not supported.  */
> 	return NO_REGS;
> 
> to:
> 
>       if (MEM_P (x) || regno < 0)
> 	{
> 	  /* In this case we can use combinations of LWC1, SWC1, LDC1 and
> 	     SDC1.  */
> 	  gcc_checking_assert (GET_MODE_SIZE (mode) % 4 == 0);
> 	  return NO_REGS;
> 	}
> 
> Please do it as a separate patch though.

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

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.

What do you think?

Regards,
Matthew