| Message ID | 1407526900.2601.48.camel@ubuntu-sellcey |
|---|---|
| State | New |
| Headers | show |
> -----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? 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. My short test indicated that the .set *float* options were being ignored if the correct command line option wasn't present. Thanks, Catherine
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? Thanks, Matthew
Matthew Fortune <matthew.fortune@imgtec.com> writes: > 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. I forgot to add that there will be some side effects to this patch which are not ideal but also unavoidable. The main one I know of is the MIPS Linux kernel which is compiled as soft-float as there must be an absolute guarantee that there is no floating point register usage. However, there is assembly code which uses the floating-point registers for context switches and these modules have not been set up to use .set hardfloat. This means that all existing kernel releases will not build with the new compiler. I have asked some of the MIPS kernel developers to apply fixes to the relevant files and back-port to any supported release. The code has always been notionally wrong but never detected as the assembler did not know that it was assembling soft-float code so allowed FP instructions. The fixed code will build with old and new tools. Similar issues will arise if anything is compiled as single-float but is using double-precision instructions currently. The fixes are simple and it is arguably an improvement to such code bases to find these issues and add appropriate .set directives to account for the special usage. Matthew
> -----Original Message----- > From: Matthew Fortune [mailto:Matthew.Fortune@imgtec.com] > Sent: Saturday, August 09, 2014 3:00 PM > To: Moore, Catherine; Steve Ellcey; echristo@gmail.com; GCC Patches > Subject: RE: [PATCH mips] Pass -msoft-float/-mhard-float flags to GAS > > 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 the example that I tried yesterday, but I am not able to reproduce today. If I notice again, I will post the example. Thanks, Catherine
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. -eric
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. 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? Matthew
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
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. */