> -----Original Message-----
> From: Andrew Pinski <pins...@gmail.com>
> Sent: Friday, November 15, 2024 7:16 PM
> To: Tamar Christina <tamar.christ...@arm.com>
> Cc: gcc-patches@gcc.gnu.org; nd <n...@arm.com>; Richard Earnshaw
> <richard.earns...@arm.com>; ktkac...@gcc.gnu.org; Richard Sandiford
> <richard.sandif...@arm.com>
> Subject: Re: [PATCH]AArch64 Suppress default options when march or mcpu used
> is not affected by it.
> 
> On Fri, Nov 15, 2024 at 6:30 AM Tamar Christina <tamar.christ...@arm.com>
> wrote:
> >
> > Hi All,
> >
> > This patch makes it so that when you use any of the Cortex-A53 errata
> > workarounds but have specified an -march or -mcpu we know is not affected by
> it
> > that we suppress the errata workaround.
> >
> > This is a driver only patch as the linker invocation needs to be changed as
> > well.  The linker and cc SPECs are different because for the linker we 
> > didn't
> > seem to add an inversion flag for the option.  That said, it's also not 
> > possible
> > to configure the linker with it on by default.  So not passing the flag is
> > sufficient to turn it off.
> >
> > For the compilers however we have an inversion flag using -mno-, which is
> needed
> > to disable the workarounds when the compiler has been configured with it by
> > default.
> >
> > Note that theoretically speaking -mcpu=native on a Cortex-A53 would turn it 
> > off,
> > but this should be ok because it's unlikely anyone is running GCC-15+ on a
> > Cortex-A53 which needs it.  If this is a concern I can adjust the patch to 
> > for
> > targets that have HAVE_LOCAL_CPU_DETECT I can make a new custom function
> that
> > re-queries host detection to see if it's an affected system.
> >
> > The workaround has the effect of suppressing certain inlining and 
> > multiply-add
> > formation which leads to about ~1% SPECCPU 2017 Intrate regression on
> modern
> > cores.  This patch is needed because most distros configure GCC with the
> > workaround enabled by default.
> >
> > I tried writing automated testcases for these, however the testsuite doesn't
> > want to scan the output of -### and it makes the excess error tests always 
> > fail
> > unless you use dg-error, which also looks for"error:".  So tested manually:
> 
> You might be able to use dg-message instead. dg-message does not look
> for a `note:` (dg-note), `error:` (dg-note) or `warning:`
> (dg-warning).
> 
> From gcc-dg.exp:
> ```
> # Look for messages that don't have standard prefixes.
> proc dg-message { args } {
> ```
> 

Thanks for the suggestion, I did try it before but both dg-output and 
dg-message fail
 test for excess errors since the -### output goes to stderr.

But I realized I misunderstood the dg-message syntax and found a way to silence 
the
excess error. So respinning the patch.

Thanks,
Tamar
> Thanks,
> Andrew Pinski
> 
> >
> > > gcc -mcpu=neoverse-v1 -mfix-cortex-a53-835769 -xc - -O3 -o - < /dev/null 
> > > -###
> 2>&1 | grep "\-mfix" | wc -l
> > 0
> >
> > > gcc -mfix-cortex-a53-835769 -xc - -O3 -o - < /dev/null -### 2>&1 | grep 
> > > "\-
> mfix" | wc -l
> > 5
> >
> > > gcc -mfix-cortex-a53-835769 -march=armv8-a -xc - -O3 -o - < /dev/null -###
> 2>&1 | grep "\-mfix" | wc -l
> > 5
> >
> > > gcc -mfix-cortex-a53-835769 -march=armv8.1-a -xc - -O3 -o - < /dev/null 
> > > -###
> 2>&1 | grep "\-mfix" | wc -l
> > 0
> >
> > > gcc -mfix-cortex-a53-835769 -march=armv8.1-a -xc - -O3 -o - < /dev/null 
> > > -###
> 2>&1 | grep "\-\-fix" | wc -l
> > 0
> >
> > > gcc -mfix-cortex-a53-835769 -march=armv8-a -xc - -O3 -o - < /dev/null -###
> 2>&1 | grep "\-\-fix" | wc -l
> > 1
> >
> > > -gcc -mfix-cortex-a53-835769 -xc - -O3 -o - < /dev/null -### 2>&1 | grep 
> > > "\-\-
> fix" | wc -l
> > 1
> >
> > Bootstrapped Regtested on aarch64-none-linux-gnu and no issues.
> >
> > Ok for master?
> >
> > Thanks,
> > Tamar
> >
> > gcc/ChangeLog:
> >
> >         * config/aarch64/aarch64-errata.h (TARGET_SUPPRESS_OPT_SPEC,
> >         TARGET_TURN_OFF_OPT_SPEC, CA53_ERR_835769_COMPILE_SPEC,
> >         CA53_ERR_843419_COMPILE_SPEC): New.
> >         (CA53_ERR_835769_SPEC, CA53_ERR_843419_SPEC): Use them.
> >         (AARCH64_ERRATA_COMPILE_SPEC):
> >         * config/aarch64/aarch64-elf-raw.h (CC1_SPEC, CC1PLUS_SPEC): Add
> >         AARCH64_ERRATA_COMPILE_SPEC.
> >         * config/aarch64/aarch64-freebsd.h (CC1_SPEC, CC1PLUS_SPEC): 
> > Likewise.
> >         * config/aarch64/aarch64-gnu.h (CC1_SPEC, CC1PLUS_SPEC): Likewise.
> >         * config/aarch64/aarch64-linux.h (CC1_SPEC, CC1PLUS_SPEC): Likewise.
> >         * config/aarch64/aarch64-netbsd.h (CC1_SPEC, CC1PLUS_SPEC): 
> > Likewise.
> >         * doc/invoke.texi: Document it.
> >
> > ---
> > diff --git a/gcc/config/aarch64/aarch64-elf-raw.h
> b/gcc/config/aarch64/aarch64-elf-raw.h
> > index
> 5396da9b2d626e23e4c4d56e19cd7aa70804c475..8442a664c4fdedd9696da90
> e6727293c4d472a3f 100644
> > --- a/gcc/config/aarch64/aarch64-elf-raw.h
> > +++ b/gcc/config/aarch64/aarch64-elf-raw.h
> > @@ -38,4 +38,12 @@
> >    AARCH64_ERRATA_LINK_SPEC
> >  #endif
> >
> > +#ifndef CC1_SPEC
> > +# define CC1_SPEC AARCH64_ERRATA_COMPILE_SPEC
> > +#endif
> > +
> > +#ifndef CC1PLUS_SPEC
> > +# define CC1PLUS_SPEC AARCH64_ERRATA_COMPILE_SPEC
> > +#endif
> > +
> >  #endif /* GCC_AARCH64_ELF_RAW_H */
> > diff --git a/gcc/config/aarch64/aarch64-errata.h 
> > b/gcc/config/aarch64/aarch64-
> errata.h
> > index
> c323595ee49553f2b3bc106e993c14f62aee235b..ac0156848abe3e7df669a7ff5
> 4e07e72e978c5f0 100644
> > --- a/gcc/config/aarch64/aarch64-errata.h
> > +++ b/gcc/config/aarch64/aarch64-errata.h
> > @@ -21,24 +21,61 @@
> >  #ifndef GCC_AARCH64_ERRATA_H
> >  #define GCC_AARCH64_ERRATA_H
> >
> > +/* Completely ignore the option if we've explicitly specify something 
> > other than
> > +   mcpu=cortex-a53 or march=armv8-a.  */
> > +#define TARGET_SUPPRESS_OPT_SPEC(OPT) \
> > +  "mcpu=*:%{!mcpu=cortex-a53:; " OPT  \
> > +  "}; march=*:%{!march=armv8-a:;" OPT "}; " OPT
> > +
> > +/* Explicitly turn off the option if we've explicitly specify something 
> > other
> > +   than mcpu=cortex-a53 or march=armv8-a.  This will also erase any other 
> > usage
> > +   of the flag making the order of the options not relevant.  */
> > +#define TARGET_TURN_OFF_OPT_SPEC(FLAG)   \
> > +  "mcpu=*:%{!mcpu=cortex-a53:%<m" FLAG " -mno-" FLAG \
> > +  "}; march=*:%{!march=armv8-a:%<m" FLAG " -mno-" FLAG "}"
> > +
> > +/* Cortex-A53 835769 Errata.  */
> > +
> >  #if TARGET_FIX_ERR_A53_835769_DEFAULT
> > -#define CA53_ERR_835769_SPEC \
> > -  " %{!mno-fix-cortex-a53-835769:--fix-cortex-a53-835769}"
> > +#define CA53_ERR_835769_SPEC     \
> > +  " %{" \
> > +  TARGET_SUPPRESS_OPT_SPEC ("!mno-fix-cortex-a53-835769:--fix-cortex-
> a53-835769") \
> > +  " }"
> >  #else
> > -#define CA53_ERR_835769_SPEC \
> > -  " %{mfix-cortex-a53-835769:--fix-cortex-a53-835769}"
> > +#define CA53_ERR_835769_SPEC     \
> > +  " %{" \
> > +  TARGET_SUPPRESS_OPT_SPEC ("mfix-cortex-a53-835769:--fix-cortex-a53-
> 835769") \
> > +  " }"
> >  #endif
> >
> > +#define CA53_ERR_835769_COMPILE_SPEC \
> > +  " %{" TARGET_TURN_OFF_OPT_SPEC ("fix-cortex-a53-835769") "}"
> > +
> > +/* Cortex-A53 843419 Errata.  */
> > +
> >  #if TARGET_FIX_ERR_A53_843419_DEFAULT
> > -#define CA53_ERR_843419_SPEC \
> > -  " %{!mno-fix-cortex-a53-843419:--fix-cortex-a53-843419}"
> > +#define CA53_ERR_843419_SPEC     \
> > +  " %{" \
> > +  TARGET_SUPPRESS_OPT_SPEC ("!mno-fix-cortex-a53-843419:--fix-cortex-
> a53-843419") \
> > +  " }"
> >  #else
> > -#define CA53_ERR_843419_SPEC \
> > -  " %{mfix-cortex-a53-843419:--fix-cortex-a53-843419}"
> > +#define CA53_ERR_843419_SPEC     \
> > +  " %{" \
> > +  TARGET_SUPPRESS_OPT_SPEC ("mfix-cortex-a53-843419:--fix-cortex-a53-
> 843419") \
> > +  " }"
> >  #endif
> >
> > +#define CA53_ERR_843419_COMPILE_SPEC \
> > +  " %{" TARGET_TURN_OFF_OPT_SPEC ("fix-cortex-a53-843419") "}"
> > +
> > +/* Exports to use in SPEC files.  */
> > +
> >  #define AARCH64_ERRATA_LINK_SPEC               \
> >    CA53_ERR_835769_SPEC                         \
> >    CA53_ERR_843419_SPEC
> >
> > +#define AARCH64_ERRATA_COMPILE_SPEC            \
> > +  CA53_ERR_835769_COMPILE_SPEC                 \
> > +  CA53_ERR_843419_COMPILE_SPEC
> > +
> >  #endif /*  GCC_AARCH64_ERRATA_H */
> > diff --git a/gcc/config/aarch64/aarch64-freebsd.h
> b/gcc/config/aarch64/aarch64-freebsd.h
> > index
> e26d69ce46c7376402c96b846e21a6c0846ebe04..8a76017538a121b4f8ce16d
> 7a64a080d84c72e25 100644
> > --- a/gcc/config/aarch64/aarch64-freebsd.h
> > +++ b/gcc/config/aarch64/aarch64-freebsd.h
> > @@ -47,6 +47,14 @@
> >      -X" SUBTARGET_EXTRA_LINK_SPEC "                             \
> >      %{mbig-endian:-EB} %{mlittle-endian:-EL}"
> >
> > +#ifndef CC1_SPEC
> > +# define CC1_SPEC AARCH64_ERRATA_COMPILE_SPEC
> > +#endif
> > +
> > +#ifndef CC1PLUS_SPEC
> > +# define CC1PLUS_SPEC AARCH64_ERRATA_COMPILE_SPEC
> > +#endif
> > +
> >  #undef  LINK_SPEC
> >  #define LINK_SPEC FBSD_TARGET_LINK_SPEC AARCH64_ERRATA_LINK_SPEC
> >
> > diff --git a/gcc/config/aarch64/aarch64-gnu.h b/gcc/config/aarch64/aarch64-
> gnu.h
> > index
> ee54940342d3000513aef28d8b29a44d1424c41b..74456263e5837ea8a53b15
> ebf632ef5407ae2ffb 100644
> > --- a/gcc/config/aarch64/aarch64-gnu.h
> > +++ b/gcc/config/aarch64/aarch64-gnu.h
> > @@ -36,6 +36,13 @@
> >     %{mbig-endian:-EB} %{mlittle-endian:-EL}     \
> >     -maarch64gnu%{mabi=ilp32:32}%{mbig-endian:b}"
> >
> > +#ifndef CC1_SPEC
> > +# define CC1_SPEC AARCH64_ERRATA_COMPILE_SPEC
> > +#endif
> > +
> > +#ifndef CC1PLUS_SPEC
> > +# define CC1PLUS_SPEC AARCH64_ERRATA_COMPILE_SPEC
> > +#endif
> >
> >  #define LINK_SPEC GNU_TARGET_LINK_SPEC AARCH64_ERRATA_LINK_SPEC
> >
> > diff --git a/gcc/config/aarch64/aarch64-linux.h 
> > b/gcc/config/aarch64/aarch64-
> linux.h
> > index
> 8e51c8202ccc87c19deb97e046ad688899680b62..7582d1734892dbbd550077
> b0f5a87e095f938ad8 100644
> > --- a/gcc/config/aarch64/aarch64-linux.h
> > +++ b/gcc/config/aarch64/aarch64-linux.h
> > @@ -29,8 +29,12 @@
> >  #undef  ASAN_CC1_SPEC
> >  #define ASAN_CC1_SPEC "%{%:sanitize(address):-funwind-tables}"
> >
> > -#undef  CC1_SPEC
> > -#define CC1_SPEC GNU_USER_TARGET_CC1_SPEC ASAN_CC1_SPEC
> > +#undef CC1_SPEC
> > +#define CC1_SPEC GNU_USER_TARGET_CC1_SPEC ASAN_CC1_SPEC \
> > +    AARCH64_ERRATA_COMPILE_SPEC
> > +
> > +#undef CC1PLUS_SPEC
> > +#define CC1PLUS_SPEC AARCH64_ERRATA_COMPILE_SPEC
> >
> >  #define CPP_SPEC "%{pthread:-D_REENTRANT}"
> >
> > diff --git a/gcc/config/aarch64/aarch64-netbsd.h
> b/gcc/config/aarch64/aarch64-netbsd.h
> > index
> fb8d10ffd18f9152740199ad8c481fe2f13f3470..57fffcd448fd976b919cf874f00
> d3341dd6f0757 100644
> > --- a/gcc/config/aarch64/aarch64-netbsd.h
> > +++ b/gcc/config/aarch64/aarch64-netbsd.h
> > @@ -39,6 +39,15 @@
> >    "%{mlittle-endian:-EL -m " TARGET_LINKER_LITTLE_EMULATION "} "       \
> >    "%(netbsd_link_spec)"
> >
> > +
> > +#ifndef CC1_SPEC
> > +# define CC1_SPEC AARCH64_ERRATA_COMPILE_SPEC
> > +#endif
> > +
> > +#ifndef CC1PLUS_SPEC
> > +# define CC1PLUS_SPEC AARCH64_ERRATA_COMPILE_SPEC
> > +#endif
> > +
> >  #undef  LINK_SPEC
> >  #define LINK_SPEC NETBSD_LINK_SPEC_ELF         \
> >                   NETBSD_TARGET_LINK_SPEC       \
> > diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> > index
> 4a494f6a668cb0e51cdf56af56c094d982812458..0280126c5c28f563b1956af8
> 74cc90cd5886d09e 100644
> > --- a/gcc/doc/invoke.texi
> > +++ b/gcc/doc/invoke.texi
> > @@ -21361,7 +21361,9 @@ This option requires binutils 2.26 or newer.
> >  @itemx -mno-fix-cortex-a53-835769
> >  Enable or disable the workaround for the ARM Cortex-A53 erratum number
> 835769.
> >  This involves inserting a NOP instruction between memory instructions and
> > -64-bit integer multiply-accumulate instructions.
> > +64-bit integer multiply-accumulate instructions.  This flag will be 
> > ignored if
> > +an architecture or cpu is specified on the commandline which does not need 
> > the
> > +workaround.
> >
> >  @opindex mfix-cortex-a53-843419
> >  @opindex mno-fix-cortex-a53-843419
> > @@ -21369,7 +21371,10 @@ This involves inserting a NOP instruction between
> memory instructions and
> >  @itemx -mno-fix-cortex-a53-843419
> >  Enable or disable the workaround for the ARM Cortex-A53 erratum number
> 843419.
> >  This erratum workaround is made at link time and this will only pass the
> > -corresponding flag to the linker.
> > +corresponding flag to the linker.  This flag will be ignored if
> > +an architecture or cpu is specified on the commandline which does not need 
> > the
> > +workaround.
> > +
> >
> >  @opindex mlow-precision-recip-sqrt
> >  @opindex mno-low-precision-recip-sqrt
> >
> >
> >
> >
> > --

Reply via email to