Re: [Patch] Add MIPS flag to avoid use of ldc1/sdc1/ldxc1/sdxc1

[Andrew Pinski <pinskia at gmail dot com>]

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