This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH][MIPS] Implement O32 FPXX ABI (GCC)
- From: Richard Sandiford <rdsandiford at googlemail dot com>
- To: Matthew Fortune <Matthew dot Fortune at imgtec dot com>
- Cc: "Joseph Myers \(joseph\ at codesourcery dot com\)" <joseph at codesourcery dot com>, "macro\ at codesourcery dot com" <macro at codesourcery dot com>, "Moore\, Catherine \(Catherine_Moore\ at mentor dot com\)" <Catherine_Moore at mentor dot com>, Rich Fuhler <Rich dot Fuhler at imgtec dot com>, "'gcc-patches\ at gcc dot gnu dot org' \(gcc-patches\ at gcc dot gnu dot org\)" <gcc-patches at gcc dot gnu dot org>
- Date: Thu, 22 May 2014 09:05:25 +0100
- Subject: Re: [PATCH][MIPS] Implement O32 FPXX ABI (GCC)
- Authentication-results: sourceware.org; auth=none
- References: <6D39441BF12EF246A7ABCE6654B023535233AE at LEMAIL01 dot le dot imgtec dot org> <87r43qlshb dot fsf at talisman dot default> <6D39441BF12EF246A7ABCE6654B02353539412 at LEMAIL01 dot le dot imgtec dot org> <87ha4ihjhk dot fsf at talisman dot default> <6D39441BF12EF246A7ABCE6654B0235353B386 at LEMAIL01 dot le dot imgtec dot org>
Matthew Fortune <Matthew.Fortune@imgtec.com> writes:
> 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.
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.
>> >> > 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.
Thanks,
Richard