| Message ID | 4f0faaab-5dd4-40c8-b66d-feb9bb515c0d@BAMAIL02.ba.imgtec.org |
|---|---|
| State | New |
| Headers | show |
On Mon, Oct 27, 2014 at 3:23 PM, Steve Ellcey <sellcey@imgtec.com> wrote: > > There are some MIPS patches that have been applied to the Google Android GCC > tree but not been submitted to FSF GCC. I would like to get those patches > checked in if possible. The first one is to add a new MIPS flag to turn > off the use of ldc1/sdc1/ldxc1/sdxc1. Google Earth has a custom allocator > that does not guarantee 8 byte alignment of the allocated memory, therefore > using these instructions (that require 8 byte alignment) could cause runtime > failures. By default, these instructions will continue to be used, the > flag is there to allow their use to be turned off if desired. That sounds like a bug in google earth's allocator if we follow either C or C++ standards when it comes to malloc/operator new. Both have to be align enough for the type which is at least that size that is being allocated. So it needs to be 8byte aligned if allocating a 8 byte object. Thanks, Andrew > > Tested on mips-mti-linux-gnu. > > OK to checkin? > > Steve Ellcey > sellcey@imgtec.com > > > > 2014-10-27 Steve Ellcey <sellcey@imgtec.com> > > * config/mips/mips.h (ISA_HAS_LDC1_SDC1): Check TARGET_LDC1_SDC1. > * config/mips/mips.md (*<ANYF:loadx>_<P:mode): Ditto. > * config/mips/mips.md (*<ANYF:storex>_<P:mode): Ditto. > * config/mips/mips.opt (mldc1-sdc1): New. > > > diff --git a/gcc/config/mips/mips.h b/gcc/config/mips/mips.h > index c7b998b..b9caff7 100644 > --- a/gcc/config/mips/mips.h > +++ b/gcc/config/mips/mips.h > @@ -892,7 +892,8 @@ struct mips_cpu_info { > /* ISA has LDC1 and SDC1. */ > #define ISA_HAS_LDC1_SDC1 (!ISA_MIPS1 \ > && !TARGET_MIPS5900 \ > - && !TARGET_MIPS16) > + && !TARGET_MIPS16 \ > + && TARGET_LDC1_SDC1) > > /* ISA has the mips4 FP condition code instructions: FP-compare to CC, > branch on CC, and move (both FP and non-FP) on CC. */ > diff --git a/gcc/config/mips/mips.md b/gcc/config/mips/mips.md > index d47bb78..77f3356 100644 > --- a/gcc/config/mips/mips.md > +++ b/gcc/config/mips/mips.md > @@ -4573,7 +4573,7 @@ > [(set (match_operand:ANYF 0 "register_operand" "=f") > (mem:ANYF (plus:P (match_operand:P 1 "register_operand" "d") > (match_operand:P 2 "register_operand" "d"))))] > - "ISA_HAS_LXC1_SXC1" > + "ISA_HAS_LXC1_SXC1 && (<MODE>mode == SFmode || TARGET_LDC1_SDC1)" > "<ANYF:loadx>\t%0,%1(%2)" > [(set_attr "type" "fpidxload") > (set_attr "mode" "<ANYF:UNITMODE>")]) > @@ -4582,7 +4582,7 @@ > [(set (mem:ANYF (plus:P (match_operand:P 1 "register_operand" "d") > (match_operand:P 2 "register_operand" "d"))) > (match_operand:ANYF 0 "register_operand" "f"))] > - "ISA_HAS_LXC1_SXC1" > + "ISA_HAS_LXC1_SXC1 && (<MODE>mode == SFmode || TARGET_LDC1_SDC1)" > "<ANYF:storex>\t%0,%1(%2)" > [(set_attr "type" "fpidxstore") > (set_attr "mode" "<ANYF:UNITMODE>")]) > diff --git a/gcc/config/mips/mips.opt b/gcc/config/mips/mips.opt > index dca4f80..7b75fc9 100644 > --- a/gcc/config/mips/mips.opt > +++ b/gcc/config/mips/mips.opt > @@ -267,6 +267,10 @@ mips3d > Target Report RejectNegative Var(TARGET_MIPS3D) > Use MIPS-3D instructions > > +mldc1-sdc1 > +Target Report Var(TARGET_LDC1_SDC1) Init(1) > +Use ldc1/ldxc1 and sdc1/sdxc1 instruction > + > mllsc > Target Report Mask(LLSC) > Use ll, sc and sync instructions
> -----Original Message----- > From: Andrew Pinski [mailto:pinskia@gmail.com] > Sent: Monday, October 27, 2014 6:32 PM > To: Steve Ellcey > Cc: Moore, Catherine; Matthew Fortune; GCC Patches > Subject: Re: [Patch] Add MIPS flag to avoid use of ldc1/sdc1/ldxc1/sdxc1 > > On Mon, Oct 27, 2014 at 3:23 PM, Steve Ellcey <sellcey@imgtec.com> wrote: > > > > There are some MIPS patches that have been applied to the Google > > Android GCC tree but not been submitted to FSF GCC. I would like to get > those patches > > checked in if possible. The first one is to add a new MIPS flag to turn > > off the use of ldc1/sdc1/ldxc1/sdxc1. Google Earth has a custom > > allocator that does not guarantee 8 byte alignment of the allocated > > memory, therefore using these instructions (that require 8 byte > > alignment) could cause runtime failures. By default, these > > instructions will continue to be used, the flag is there to allow their use to > be turned off if desired. > > That sounds like a bug in google earth's allocator if we follow either C or C++ > standards when it comes to malloc/operator new. > Both have to be align enough for the type which is at least that size that is > being allocated. So it needs to be 8byte aligned if allocating a 8 byte object. > Andrew, Do you have an objection to allowing an option to disable these instructions (despite the reason for wanting to do so)? Thanks, Catherine > > > > > Tested on mips-mti-linux-gnu. > > > > OK to checkin? > > > > Steve Ellcey > > sellcey@imgtec.com > > > > > > > > 2014-10-27 Steve Ellcey <sellcey@imgtec.com> > > > > * config/mips/mips.h (ISA_HAS_LDC1_SDC1): Check > TARGET_LDC1_SDC1. > > * config/mips/mips.md (*<ANYF:loadx>_<P:mode): Ditto. > > * config/mips/mips.md (*<ANYF:storex>_<P:mode): Ditto. > > * config/mips/mips.opt (mldc1-sdc1): New. > > > > > > diff --git a/gcc/config/mips/mips.h b/gcc/config/mips/mips.h index > > c7b998b..b9caff7 100644 > > --- a/gcc/config/mips/mips.h > > +++ b/gcc/config/mips/mips.h > > @@ -892,7 +892,8 @@ struct mips_cpu_info { > > /* ISA has LDC1 and SDC1. */ > > #define ISA_HAS_LDC1_SDC1 (!ISA_MIPS1 \ > > && !TARGET_MIPS5900 \ > > - && !TARGET_MIPS16) > > + && !TARGET_MIPS16 \ > > + && TARGET_LDC1_SDC1) > > > > /* ISA has the mips4 FP condition code instructions: FP-compare to CC, > > branch on CC, and move (both FP and non-FP) on CC. */ diff --git > > a/gcc/config/mips/mips.md b/gcc/config/mips/mips.md index > > d47bb78..77f3356 100644 > > --- a/gcc/config/mips/mips.md > > +++ b/gcc/config/mips/mips.md > > @@ -4573,7 +4573,7 @@ > > [(set (match_operand:ANYF 0 "register_operand" "=f") > > (mem:ANYF (plus:P (match_operand:P 1 "register_operand" "d") > > (match_operand:P 2 "register_operand" > > "d"))))] > > - "ISA_HAS_LXC1_SXC1" > > + "ISA_HAS_LXC1_SXC1 && (<MODE>mode == SFmode || > TARGET_LDC1_SDC1)" > > "<ANYF:loadx>\t%0,%1(%2)" > > [(set_attr "type" "fpidxload") > > (set_attr "mode" "<ANYF:UNITMODE>")]) @@ -4582,7 +4582,7 @@ > > [(set (mem:ANYF (plus:P (match_operand:P 1 "register_operand" "d") > > (match_operand:P 2 "register_operand" "d"))) > > (match_operand:ANYF 0 "register_operand" "f"))] > > - "ISA_HAS_LXC1_SXC1" > > + "ISA_HAS_LXC1_SXC1 && (<MODE>mode == SFmode || > TARGET_LDC1_SDC1)" > > "<ANYF:storex>\t%0,%1(%2)" > > [(set_attr "type" "fpidxstore") > > (set_attr "mode" "<ANYF:UNITMODE>")]) diff --git > > a/gcc/config/mips/mips.opt b/gcc/config/mips/mips.opt index > > dca4f80..7b75fc9 100644 > > --- a/gcc/config/mips/mips.opt > > +++ b/gcc/config/mips/mips.opt > > @@ -267,6 +267,10 @@ mips3d > > Target Report RejectNegative Var(TARGET_MIPS3D) Use MIPS-3D > > instructions > > > > +mldc1-sdc1 > > +Target Report Var(TARGET_LDC1_SDC1) Init(1) Use ldc1/ldxc1 and > > +sdc1/sdxc1 instruction > > + > > mllsc > > Target Report Mask(LLSC) > > Use ll, sc and sync instructions
On Mon, 2014-10-27 at 15:32 -0700, Andrew Pinski wrote: > On Mon, Oct 27, 2014 at 3:23 PM, Steve Ellcey <sellcey@imgtec.com> wrote: > > > > There are some MIPS patches that have been applied to the Google Android GCC > > tree but not been submitted to FSF GCC. I would like to get those patches > > checked in if possible. The first one is to add a new MIPS flag to turn > > off the use of ldc1/sdc1/ldxc1/sdxc1. Google Earth has a custom allocator > > that does not guarantee 8 byte alignment of the allocated memory, therefore > > using these instructions (that require 8 byte alignment) could cause runtime > > failures. By default, these instructions will continue to be used, the > > flag is there to allow their use to be turned off if desired. > > That sounds like a bug in google earth's allocator if we follow either > C or C++ standards when it comes to malloc/operator new. > Both have to be align enough for the type which is at least that size > that is being allocated. So it needs to be 8byte aligned if > allocating a 8 byte object. > > Thanks, > Andrew I agree that it is a bug in google earth's allocator. I just don't think I can get them to change it so I need some way to deal with the problem. Steve Ellcey
On Mon, Oct 27, 2014 at 3:35 PM, Moore, Catherine <Catherine_Moore@mentor.com> wrote: > > >> -----Original Message----- >> From: Andrew Pinski [mailto:pinskia@gmail.com] >> Sent: Monday, October 27, 2014 6:32 PM >> To: Steve Ellcey >> Cc: Moore, Catherine; Matthew Fortune; GCC Patches >> Subject: Re: [Patch] Add MIPS flag to avoid use of ldc1/sdc1/ldxc1/sdxc1 >> >> On Mon, Oct 27, 2014 at 3:23 PM, Steve Ellcey <sellcey@imgtec.com> wrote: >> > >> > There are some MIPS patches that have been applied to the Google >> > Android GCC tree but not been submitted to FSF GCC. I would like to get >> those patches >> > checked in if possible. The first one is to add a new MIPS flag to turn >> > off the use of ldc1/sdc1/ldxc1/sdxc1. Google Earth has a custom >> > allocator that does not guarantee 8 byte alignment of the allocated >> > memory, therefore using these instructions (that require 8 byte >> > alignment) could cause runtime failures. By default, these >> > instructions will continue to be used, the flag is there to allow their use to >> be turned off if desired. >> >> That sounds like a bug in google earth's allocator if we follow either C or C++ >> standards when it comes to malloc/operator new. >> Both have to be align enough for the type which is at least that size that is >> being allocated. So it needs to be 8byte aligned if allocating a 8 byte object. >> > > Andrew, > > Do you have an objection to allowing an option to disable these instructions (despite the reason for wanting to do so)? Yes this seems like a bad workaround for broken code. Thanks, Andrew Pinski > > Thanks, > Catherine > >> >> > >> > Tested on mips-mti-linux-gnu. >> > >> > OK to checkin? >> > >> > Steve Ellcey >> > sellcey@imgtec.com >> > >> > >> > >> > 2014-10-27 Steve Ellcey <sellcey@imgtec.com> >> > >> > * config/mips/mips.h (ISA_HAS_LDC1_SDC1): Check >> TARGET_LDC1_SDC1. >> > * config/mips/mips.md (*<ANYF:loadx>_<P:mode): Ditto. >> > * config/mips/mips.md (*<ANYF:storex>_<P:mode): Ditto. >> > * config/mips/mips.opt (mldc1-sdc1): New. >> > >> > >> > diff --git a/gcc/config/mips/mips.h b/gcc/config/mips/mips.h index >> > c7b998b..b9caff7 100644 >> > --- a/gcc/config/mips/mips.h >> > +++ b/gcc/config/mips/mips.h >> > @@ -892,7 +892,8 @@ struct mips_cpu_info { >> > /* ISA has LDC1 and SDC1. */ >> > #define ISA_HAS_LDC1_SDC1 (!ISA_MIPS1 \ >> > && !TARGET_MIPS5900 \ >> > - && !TARGET_MIPS16) >> > + && !TARGET_MIPS16 \ >> > + && TARGET_LDC1_SDC1) >> > >> > /* ISA has the mips4 FP condition code instructions: FP-compare to CC, >> > branch on CC, and move (both FP and non-FP) on CC. */ diff --git >> > a/gcc/config/mips/mips.md b/gcc/config/mips/mips.md index >> > d47bb78..77f3356 100644 >> > --- a/gcc/config/mips/mips.md >> > +++ b/gcc/config/mips/mips.md >> > @@ -4573,7 +4573,7 @@ >> > [(set (match_operand:ANYF 0 "register_operand" "=f") >> > (mem:ANYF (plus:P (match_operand:P 1 "register_operand" "d") >> > (match_operand:P 2 "register_operand" >> > "d"))))] >> > - "ISA_HAS_LXC1_SXC1" >> > + "ISA_HAS_LXC1_SXC1 && (<MODE>mode == SFmode || >> TARGET_LDC1_SDC1)" >> > "<ANYF:loadx>\t%0,%1(%2)" >> > [(set_attr "type" "fpidxload") >> > (set_attr "mode" "<ANYF:UNITMODE>")]) @@ -4582,7 +4582,7 @@ >> > [(set (mem:ANYF (plus:P (match_operand:P 1 "register_operand" "d") >> > (match_operand:P 2 "register_operand" "d"))) >> > (match_operand:ANYF 0 "register_operand" "f"))] >> > - "ISA_HAS_LXC1_SXC1" >> > + "ISA_HAS_LXC1_SXC1 && (<MODE>mode == SFmode || >> TARGET_LDC1_SDC1)" >> > "<ANYF:storex>\t%0,%1(%2)" >> > [(set_attr "type" "fpidxstore") >> > (set_attr "mode" "<ANYF:UNITMODE>")]) diff --git >> > a/gcc/config/mips/mips.opt b/gcc/config/mips/mips.opt index >> > dca4f80..7b75fc9 100644 >> > --- a/gcc/config/mips/mips.opt >> > +++ b/gcc/config/mips/mips.opt >> > @@ -267,6 +267,10 @@ mips3d >> > Target Report RejectNegative Var(TARGET_MIPS3D) Use MIPS-3D >> > instructions >> > >> > +mldc1-sdc1 >> > +Target Report Var(TARGET_LDC1_SDC1) Init(1) Use ldc1/ldxc1 and >> > +sdc1/sdxc1 instruction >> > + >> > mllsc >> > Target Report Mask(LLSC) >> > Use ll, sc and sync instructions
New command-line options need documenting in invoke.texi.
> -----Original Message----- > From: Andrew Pinski [mailto:pinskia@gmail.com] > Sent: Monday, October 27, 2014 6:41 PM > To: Moore, Catherine > Cc: Steve Ellcey; Matthew Fortune; GCC Patches > Subject: Re: [Patch] Add MIPS flag to avoid use of ldc1/sdc1/ldxc1/sdxc1 > > On Mon, Oct 27, 2014 at 3:35 PM, Moore, Catherine > <Catherine_Moore@mentor.com> wrote: > > > > > >> -----Original Message----- > >> From: Andrew Pinski [mailto:pinskia@gmail.com] > >> Sent: Monday, October 27, 2014 6:32 PM > >> To: Steve Ellcey > >> Cc: Moore, Catherine; Matthew Fortune; GCC Patches > >> Subject: Re: [Patch] Add MIPS flag to avoid use of > >> ldc1/sdc1/ldxc1/sdxc1 > >> > >> On Mon, Oct 27, 2014 at 3:23 PM, Steve Ellcey <sellcey@imgtec.com> > wrote: > >> > > >> > There are some MIPS patches that have been applied to the Google > >> > Android GCC tree but not been submitted to FSF GCC. I would like > >> > to get > >> those patches > >> > checked in if possible. The first one is to add a new MIPS flag to turn > >> > off the use of ldc1/sdc1/ldxc1/sdxc1. Google Earth has a custom > >> > allocator that does not guarantee 8 byte alignment of the allocated > >> > memory, therefore using these instructions (that require 8 byte > >> > alignment) could cause runtime failures. By default, these > >> > instructions will continue to be used, the flag is there to allow > >> > their use to > >> be turned off if desired. > >> > >> That sounds like a bug in google earth's allocator if we follow > >> either C or C++ standards when it comes to malloc/operator new. > >> Both have to be align enough for the type which is at least that size > >> that is being allocated. So it needs to be 8byte aligned if allocating a 8 byte > object. > >> > > > > Andrew, > > > > Do you have an objection to allowing an option to disable these > instructions (despite the reason for wanting to do so)? > > Yes this seems like a bad workaround for broken code. > Well, we work around broken hardware all the time. What would you suggest that Steve do instead? Should we tell him to add an -mfix-google-earth option? All kidding aside, I wouldn't mind a viable suggestion. > > > >> > >> > > >> > Tested on mips-mti-linux-gnu. > >> > > >> > OK to checkin? > >> > > >> > Steve Ellcey > >> > sellcey@imgtec.com > >> > > >> > > >> > > >> > 2014-10-27 Steve Ellcey <sellcey@imgtec.com> > >> > > >> > * config/mips/mips.h (ISA_HAS_LDC1_SDC1): Check > >> TARGET_LDC1_SDC1. > >> > * config/mips/mips.md (*<ANYF:loadx>_<P:mode): Ditto. > >> > * config/mips/mips.md (*<ANYF:storex>_<P:mode): Ditto. > >> > * config/mips/mips.opt (mldc1-sdc1): New. > >> > > >> > > >> > diff --git a/gcc/config/mips/mips.h b/gcc/config/mips/mips.h index > >> > c7b998b..b9caff7 100644 > >> > --- a/gcc/config/mips/mips.h > >> > +++ b/gcc/config/mips/mips.h > >> > @@ -892,7 +892,8 @@ struct mips_cpu_info { > >> > /* ISA has LDC1 and SDC1. */ > >> > #define ISA_HAS_LDC1_SDC1 (!ISA_MIPS1 \ > >> > && !TARGET_MIPS5900 \ > >> > - && !TARGET_MIPS16) > >> > + && !TARGET_MIPS16 \ > >> > + && TARGET_LDC1_SDC1) > >> > > >> > /* ISA has the mips4 FP condition code instructions: FP-compare to CC, > >> > branch on CC, and move (both FP and non-FP) on CC. */ diff > >> > --git a/gcc/config/mips/mips.md b/gcc/config/mips/mips.md index > >> > d47bb78..77f3356 100644 > >> > --- a/gcc/config/mips/mips.md > >> > +++ b/gcc/config/mips/mips.md > >> > @@ -4573,7 +4573,7 @@ > >> > [(set (match_operand:ANYF 0 "register_operand" "=f") > >> > (mem:ANYF (plus:P (match_operand:P 1 "register_operand" "d") > >> > (match_operand:P 2 "register_operand" > >> > "d"))))] > >> > - "ISA_HAS_LXC1_SXC1" > >> > + "ISA_HAS_LXC1_SXC1 && (<MODE>mode == SFmode || > >> TARGET_LDC1_SDC1)" > >> > "<ANYF:loadx>\t%0,%1(%2)" > >> > [(set_attr "type" "fpidxload") > >> > (set_attr "mode" "<ANYF:UNITMODE>")]) @@ -4582,7 +4582,7 @@ > >> > [(set (mem:ANYF (plus:P (match_operand:P 1 "register_operand" "d") > >> > (match_operand:P 2 "register_operand" "d"))) > >> > (match_operand:ANYF 0 "register_operand" "f"))] > >> > - "ISA_HAS_LXC1_SXC1" > >> > + "ISA_HAS_LXC1_SXC1 && (<MODE>mode == SFmode || > >> TARGET_LDC1_SDC1)" > >> > "<ANYF:storex>\t%0,%1(%2)" > >> > [(set_attr "type" "fpidxstore") > >> > (set_attr "mode" "<ANYF:UNITMODE>")]) diff --git > >> > a/gcc/config/mips/mips.opt b/gcc/config/mips/mips.opt index > >> > dca4f80..7b75fc9 100644 > >> > --- a/gcc/config/mips/mips.opt > >> > +++ b/gcc/config/mips/mips.opt > >> > @@ -267,6 +267,10 @@ mips3d > >> > Target Report RejectNegative Var(TARGET_MIPS3D) Use MIPS-3D > >> > instructions > >> > > >> > +mldc1-sdc1 > >> > +Target Report Var(TARGET_LDC1_SDC1) Init(1) Use ldc1/ldxc1 and > >> > +sdc1/sdxc1 instruction > >> > + > >> > mllsc > >> > Target Report Mask(LLSC) > >> > Use ll, sc and sync instructions
On Mon, Oct 27, 2014 at 3:44 PM, Moore, Catherine <Catherine_Moore@mentor.com> wrote: > > >> -----Original Message----- >> From: Andrew Pinski [mailto:pinskia@gmail.com] >> Sent: Monday, October 27, 2014 6:41 PM >> To: Moore, Catherine >> Cc: Steve Ellcey; Matthew Fortune; GCC Patches >> Subject: Re: [Patch] Add MIPS flag to avoid use of ldc1/sdc1/ldxc1/sdxc1 >> >> On Mon, Oct 27, 2014 at 3:35 PM, Moore, Catherine >> <Catherine_Moore@mentor.com> wrote: >> > >> > >> >> -----Original Message----- >> >> From: Andrew Pinski [mailto:pinskia@gmail.com] >> >> Sent: Monday, October 27, 2014 6:32 PM >> >> To: Steve Ellcey >> >> Cc: Moore, Catherine; Matthew Fortune; GCC Patches >> >> Subject: Re: [Patch] Add MIPS flag to avoid use of >> >> ldc1/sdc1/ldxc1/sdxc1 >> >> >> >> On Mon, Oct 27, 2014 at 3:23 PM, Steve Ellcey <sellcey@imgtec.com> >> wrote: >> >> > >> >> > There are some MIPS patches that have been applied to the Google >> >> > Android GCC tree but not been submitted to FSF GCC. I would like >> >> > to get >> >> those patches >> >> > checked in if possible. The first one is to add a new MIPS flag to turn >> >> > off the use of ldc1/sdc1/ldxc1/sdxc1. Google Earth has a custom >> >> > allocator that does not guarantee 8 byte alignment of the allocated >> >> > memory, therefore using these instructions (that require 8 byte >> >> > alignment) could cause runtime failures. By default, these >> >> > instructions will continue to be used, the flag is there to allow >> >> > their use to >> >> be turned off if desired. >> >> >> >> That sounds like a bug in google earth's allocator if we follow >> >> either C or C++ standards when it comes to malloc/operator new. >> >> Both have to be align enough for the type which is at least that size >> >> that is being allocated. So it needs to be 8byte aligned if allocating a 8 byte >> object. >> >> >> > >> > Andrew, >> > >> > Do you have an objection to allowing an option to disable these >> instructions (despite the reason for wanting to do so)? >> >> Yes this seems like a bad workaround for broken code. >> > Well, we work around broken hardware all the time. What would you suggest that Steve do instead? We work around broken hardware all the time yes but that is something which is hard to change. Software is easier to fix. That does not mean in the compiler. The compiler team should not get themselves in the business of working around broken software that depends on undefined behavior (this undefined behavior is very obvious and easier to fix in the source of the problem). Report the bug to Google. Thanks, Andrew Pinski > Should we tell him to add an -mfix-google-earth option? All kidding aside, I wouldn't mind a viable suggestion. > > > > >> > >> >> >> >> > >> >> > Tested on mips-mti-linux-gnu. >> >> > >> >> > OK to checkin? >> >> > >> >> > Steve Ellcey >> >> > sellcey@imgtec.com >> >> > >> >> > >> >> > >> >> > 2014-10-27 Steve Ellcey <sellcey@imgtec.com> >> >> > >> >> > * config/mips/mips.h (ISA_HAS_LDC1_SDC1): Check >> >> TARGET_LDC1_SDC1. >> >> > * config/mips/mips.md (*<ANYF:loadx>_<P:mode): Ditto. >> >> > * config/mips/mips.md (*<ANYF:storex>_<P:mode): Ditto. >> >> > * config/mips/mips.opt (mldc1-sdc1): New. >> >> > >> >> > >> >> > diff --git a/gcc/config/mips/mips.h b/gcc/config/mips/mips.h index >> >> > c7b998b..b9caff7 100644 >> >> > --- a/gcc/config/mips/mips.h >> >> > +++ b/gcc/config/mips/mips.h >> >> > @@ -892,7 +892,8 @@ struct mips_cpu_info { >> >> > /* ISA has LDC1 and SDC1. */ >> >> > #define ISA_HAS_LDC1_SDC1 (!ISA_MIPS1 \ >> >> > && !TARGET_MIPS5900 \ >> >> > - && !TARGET_MIPS16) >> >> > + && !TARGET_MIPS16 \ >> >> > + && TARGET_LDC1_SDC1) >> >> > >> >> > /* ISA has the mips4 FP condition code instructions: FP-compare to CC, >> >> > branch on CC, and move (both FP and non-FP) on CC. */ diff >> >> > --git a/gcc/config/mips/mips.md b/gcc/config/mips/mips.md index >> >> > d47bb78..77f3356 100644 >> >> > --- a/gcc/config/mips/mips.md >> >> > +++ b/gcc/config/mips/mips.md >> >> > @@ -4573,7 +4573,7 @@ >> >> > [(set (match_operand:ANYF 0 "register_operand" "=f") >> >> > (mem:ANYF (plus:P (match_operand:P 1 "register_operand" "d") >> >> > (match_operand:P 2 "register_operand" >> >> > "d"))))] >> >> > - "ISA_HAS_LXC1_SXC1" >> >> > + "ISA_HAS_LXC1_SXC1 && (<MODE>mode == SFmode || >> >> TARGET_LDC1_SDC1)" >> >> > "<ANYF:loadx>\t%0,%1(%2)" >> >> > [(set_attr "type" "fpidxload") >> >> > (set_attr "mode" "<ANYF:UNITMODE>")]) @@ -4582,7 +4582,7 @@ >> >> > [(set (mem:ANYF (plus:P (match_operand:P 1 "register_operand" "d") >> >> > (match_operand:P 2 "register_operand" "d"))) >> >> > (match_operand:ANYF 0 "register_operand" "f"))] >> >> > - "ISA_HAS_LXC1_SXC1" >> >> > + "ISA_HAS_LXC1_SXC1 && (<MODE>mode == SFmode || >> >> TARGET_LDC1_SDC1)" >> >> > "<ANYF:storex>\t%0,%1(%2)" >> >> > [(set_attr "type" "fpidxstore") >> >> > (set_attr "mode" "<ANYF:UNITMODE>")]) diff --git >> >> > a/gcc/config/mips/mips.opt b/gcc/config/mips/mips.opt index >> >> > dca4f80..7b75fc9 100644 >> >> > --- a/gcc/config/mips/mips.opt >> >> > +++ b/gcc/config/mips/mips.opt >> >> > @@ -267,6 +267,10 @@ mips3d >> >> > Target Report RejectNegative Var(TARGET_MIPS3D) Use MIPS-3D >> >> > instructions >> >> > >> >> > +mldc1-sdc1 >> >> > +Target Report Var(TARGET_LDC1_SDC1) Init(1) Use ldc1/ldxc1 and >> >> > +sdc1/sdxc1 instruction >> >> > + >> >> > mllsc >> >> > Target Report Mask(LLSC) >> >> > Use ll, sc and sync instructions
> >> > Do you have an objection to allowing an option to disable these > >> instructions (despite the reason for wanting to do so)? > >> > >> Yes this seems like a bad workaround for broken code. > >> > > Well, we work around broken hardware all the time. What would you > suggest that Steve do instead? > > We work around broken hardware all the time yes but that is something > which is hard to change. Software is easier to fix. That does not > mean in the compiler. The compiler team should not get themselves in > the business of working around broken software that depends on > undefined behavior (this undefined behavior is very obvious and easier > to fix in the source of the problem). Report the bug to Google. I'm mostly in agreement with Andrew on this. I'd like to understand more about the use-case and what happens for other architectures (only ARM and x86 are really relevant here I guess). I believe x86 will just happily load/store doubles to 4-byte aligned addresses but I'm not sure for ARM. If ARM has to take unaligned access faults for this then I think MIPS simply has to do the same. Unless the application is allocating thousands of small chunks of data and the padding is significant in the overall memory usage then I'd have thought the allocator could be fixed relatively easily. I'd also expect that to be generally a performance win for the application on all architectures. 'If' we were to make a change to the compiler then I would make it a little more generic/focus on the actual issue which relates to alignment. I'm not sure what I would call the option but its effect would be for the compiler to not rely on any greater than 'X' alignment and disable the use of any load/store instructions which need larger alignment. That still feels like a bad feature though. Matthew
On Tue, Oct 28, 2014 at 11:46 AM, Matthew Fortune <Matthew.Fortune@imgtec.com> wrote: >> >> > Do you have an objection to allowing an option to disable these >> >> instructions (despite the reason for wanting to do so)? >> >> >> >> Yes this seems like a bad workaround for broken code. >> >> >> > Well, we work around broken hardware all the time. What would you >> suggest that Steve do instead? >> >> We work around broken hardware all the time yes but that is something >> which is hard to change. Software is easier to fix. That does not >> mean in the compiler. The compiler team should not get themselves in >> the business of working around broken software that depends on >> undefined behavior (this undefined behavior is very obvious and easier >> to fix in the source of the problem). Report the bug to Google. > > I'm mostly in agreement with Andrew on this. I'd like to understand more > about the use-case and what happens for other architectures (only ARM > and x86 are really relevant here I guess). I believe x86 will just > happily load/store doubles to 4-byte aligned addresses but I'm not sure > for ARM. If ARM has to take unaligned access faults for this then I > think MIPS simply has to do the same. On AArch32 (ARM) the equivalent general purpose instructions (ldrd / strd , vldr / vstr, vpush / vpop, vldm / vstm) require only 4 byte alignment in the architecture. The only case where we may have a problem is with ldrexd / strexd instructions which are used for atomics but I'm not sure if that is under question / discussion here. This is really an application "design / portability" issue rather than anything else. So, there's no need to take alignment faults on AArch32 in this situation. Oh and before someone asks AArch64 is not a strict alignment target and so the compiler is free to emit unaligned accesses unless it is told not to do so. regards Ramana
Matthew Fortune <matthew.fortune@imgtec.com> writes: > > >> > Do you have an objection to allowing an option to disable these > > >> instructions (despite the reason for wanting to do so)? > > >> > > >> Yes this seems like a bad workaround for broken code. > > >> > > > Well, we work around broken hardware all the time. What would you > > suggest that Steve do instead? > > > > We work around broken hardware all the time yes but that is something > > which is hard to change. Software is easier to fix. That does not > > mean in the compiler. The compiler team should not get themselves in > > the business of working around broken software that depends on > > undefined behavior (this undefined behavior is very obvious and easier > > to fix in the source of the problem). Report the bug to Google. > > I'm mostly in agreement with Andrew on this. I'd like to understand more > about the use-case and what happens for other architectures (only ARM > and x86 are really relevant here I guess). I believe x86 will just > happily load/store doubles to 4-byte aligned addresses but I'm not sure > for ARM. If ARM has to take unaligned access faults for this then I > think MIPS simply has to do the same. > > Unless the application is allocating thousands of small chunks of data > and the padding is significant in the overall memory usage then I'd have > thought the allocator could be fixed relatively easily. I'd also expect > that to be generally a performance win for the application on all > architectures. > > 'If' we were to make a change to the compiler then I would make it a > little more generic/focus on the actual issue which relates to alignment. > I'm not sure what I would call the option but its effect would be for > the compiler to not rely on any greater than 'X' alignment and disable > the use of any load/store instructions which need larger alignment. > > That still feels like a bad feature though. Opinions aside there are actually some technical problems with this feature. In particular we have to consider that the o32 ABI is about to start its long awaited transition to use 64-bit floating-point registers. We know that removing LDC1/SDC1 is possible for the original O32 FP32 ABI as doubles just get handled with pairs of LWC1/SWC1. Now if we consider O32 FP64 then it is not possible to use LWC1 to access the upper-32-bits of doubles but we do have MTHC1/MFHC1 so the data would have to move via GPRs which doubles the penalty in a way as GPR-FPR transfers are generally slow. (I think the patch as it stands will fail if used with O32 FP64 but I haven't looked in detail) The bigger problem comes from the transitional ABI O32 FPXX. Firstly, O32 FPXX on MIPS32r2 onwards... this will be OK as it has MTHC1/MFHC1 so can use the same solution as O32 FP64. Next, O32 FPXX on MIPS II... this is simply impossible, there is no way to load a double-precision FPR given the constraints of the O32 FPXX ABI and the removal of LDC1/SDC1. While the patch could be fixed to account for all of this we need to determine if the case which won't work is the exact one which is needed (and I believe it is). I.e. Android applications for MIPS are MIPS32r1 O32 FP32 NAN1985 currently and will transition from FP32 to FPXX to enable the use of things like MSA. Since MIPS32r1 does not have MTHC1/MFHC1 then this solution will not work. We'll have to talk with the google engineers to explain the issues. Matthew
On Tue, 2014-10-28 at 14:06 -0700, Matthew Fortune wrote: > While the patch could be fixed to account for all of this we need to > determine if the case which won't work is the exact one which is needed > (and I believe it is). I.e. Android applications for MIPS are MIPS32r1 > O32 FP32 NAN1985 currently and will transition from FP32 to FPXX to > enable the use of things like MSA. Since MIPS32r1 does not have > MTHC1/MFHC1 then this solution will not work. > > We'll have to talk with the google engineers to explain the issues. > > Matthew FYI: I have emailed my Google contact to see if they could change their allocator and included a reference to section 7.20.3 of the C standard. Section 7.20.3 of C99 states: The pointer returned if the allocation succeeds is suitably aligned so that it may be assigned to a pointer to any type of object. Steve Ellcey
diff --git a/gcc/config/mips/mips.h b/gcc/config/mips/mips.h index c7b998b..b9caff7 100644 --- a/gcc/config/mips/mips.h +++ b/gcc/config/mips/mips.h @@ -892,7 +892,8 @@ struct mips_cpu_info { /* ISA has LDC1 and SDC1. */ #define ISA_HAS_LDC1_SDC1 (!ISA_MIPS1 \ && !TARGET_MIPS5900 \ - && !TARGET_MIPS16) + && !TARGET_MIPS16 \ + && TARGET_LDC1_SDC1) /* ISA has the mips4 FP condition code instructions: FP-compare to CC, branch on CC, and move (both FP and non-FP) on CC. */ diff --git a/gcc/config/mips/mips.md b/gcc/config/mips/mips.md index d47bb78..77f3356 100644 --- a/gcc/config/mips/mips.md +++ b/gcc/config/mips/mips.md @@ -4573,7 +4573,7 @@ [(set (match_operand:ANYF 0 "register_operand" "=f") (mem:ANYF (plus:P (match_operand:P 1 "register_operand" "d") (match_operand:P 2 "register_operand" "d"))))] - "ISA_HAS_LXC1_SXC1" + "ISA_HAS_LXC1_SXC1 && (<MODE>mode == SFmode || TARGET_LDC1_SDC1)" "<ANYF:loadx>\t%0,%1(%2)" [(set_attr "type" "fpidxload") (set_attr "mode" "<ANYF:UNITMODE>")]) @@ -4582,7 +4582,7 @@ [(set (mem:ANYF (plus:P (match_operand:P 1 "register_operand" "d") (match_operand:P 2 "register_operand" "d"))) (match_operand:ANYF 0 "register_operand" "f"))] - "ISA_HAS_LXC1_SXC1" + "ISA_HAS_LXC1_SXC1 && (<MODE>mode == SFmode || TARGET_LDC1_SDC1)" "<ANYF:storex>\t%0,%1(%2)" [(set_attr "type" "fpidxstore") (set_attr "mode" "<ANYF:UNITMODE>")]) diff --git a/gcc/config/mips/mips.opt b/gcc/config/mips/mips.opt index dca4f80..7b75fc9 100644 --- a/gcc/config/mips/mips.opt +++ b/gcc/config/mips/mips.opt @@ -267,6 +267,10 @@ mips3d Target Report RejectNegative Var(TARGET_MIPS3D) Use MIPS-3D instructions +mldc1-sdc1 +Target Report Var(TARGET_LDC1_SDC1) Init(1) +Use ldc1/ldxc1 and sdc1/sdxc1 instruction + mllsc Target Report Mask(LLSC) Use ll, sc and sync instructions