Re: [PATCH mips] Pass -msoft-float/-mhard-float flags to GAS

[Eric Christopher <echristo at gmail dot com>]

On Tue, Aug 12, 2014 at 2:07 AM, Matthew Fortune
<Matthew.Fortune@imgtec.com> wrote:
> Eric Christopher <echristo@gmail.com> writes:
>> On Sat, Aug 9, 2014 at 12:00 PM, Matthew Fortune
>> <Matthew.Fortune@imgtec.com> wrote:
>> > Moore, Catherine <Catherine_Moore@mentor.com> writes:
>> >> > -----Original Message-----
>> >> > From: Steve Ellcey [mailto:sellcey@mips.com]
>> >> > Sent: Friday, August 08, 2014 3:42 PM
>> >> > To: Moore, Catherine; matthew.fortune@imgtec.com; echristo@gmail.com;
>> >> >
>> >> > 2014-08-08  Steve Ellcey  <sellcey@mips.com>
>> >> >
>> >> >     * config/mips/mips.h (ASM_SPEC): Pass float options to assembler.
>> >> >
>> >> > diff --git a/gcc/config/mips/mips.h b/gcc/config/mips/mips.h index
>> >> > 8d7a09f..9a15287 100644
>> >> > --- a/gcc/config/mips/mips.h
>> >> > +++ b/gcc/config/mips/mips.h
>> >> > @@ -1187,6 +1187,8 @@ struct mips_cpu_info {  %{mshared} %{mno-
>> >> > shared} \  %{msym32} %{mno-sym32} \  %{mtune=*} \
>> >> > +%{mhard-float} %{msoft-float} \
>> >> > +%{msingle-float} %{mdouble-float} \
>> >> >  %(subtarget_asm_spec)"
>> >> >
>> >> >  /* Extra switches sometimes passed to the linker.  */
>> >> >
>> >>
>> >> Hi Steve,
>> >> The patch itself looks okay, but perhaps a question for Matthew.
>> >> Does the fact that the assembler requires -msoft-float even if .set
>> >> softfloat is present in the .s file deliberate behavior?
>> >
>> > The assembler requires -msoft-float if .gnu_attribute 4,3 is given. I.e.
>> the
>> > overall module options must match the ABI which has been specified. .set
>> > directives can still be used to override the 'current' options and be
>> > inconsistent with the overall module and/or .gnu_attribute setting.
>> >
>> >> I don't have a problem with passing along the *float* options to gas,
>> but
>> >> would hope that the .set options were honored as well.
>> >
>> > Yes they should be.
>> >
>> >> My short test indicated that the .set *float* options were being ignored
>> if
>> >> the correct command line option wasn't present.
>> >
>> > I'm not certain what you are describing here. Could you confirm with an
>> example
>> > just in case something is not working as expected?
>> >
>>
>> I don't have one offhand, but in skimming the binutils patch I don't
>> recall seeing anything that tested this combination. May have missed
>> it though.
>>
>> That said, the patch looks ok and if you'd like to add some tests for
>> .set and the command line options to binutils that'd be great as well.
>
> What sort of coverage do you think we need here? I did exhaustive coverage
> of anything which can affect the ABI but don't think we need the same for
> command-line options vs .module vs .set.
>

I probably would, but that's a different review thread the the patch
is already in so I'm not going to push it here :)

> There were two pre-existing tests: mips-hard-float-flag and
> mips-double-float-flag which pretty much test these cases I think. Is that
> OK for now until we identify any other areas which need additional
> coverage?

Sure.

-eric