Hi Kyril,
Thanks for the review, unfortunately this is an old version of the patch, I
sent a new one on Thu 11/21/2024
with updates and automated tests.
Would you mind reviewing that one? I've noted the documentation comment you
mentioned :)
Thanks,
Tamar
> -----Original Message-----
> From: Kyrylo Tkachov <ktkac...@nvidia.com>
> Sent: Friday, November 29, 2024 11:21 AM
> To: Tamar Christina <tamar.christ...@arm.com>
> Cc: GCC Patches <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.
>
> Hi Tamar,
>
> > On 15 Nov 2024, at 14:24, 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:
> >
> >> 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.
>
> Let’s use “command line” with a space.
> Ok.
> Thanks,
> Kyrill
>
>
> > +
> >
> > @opindex mlow-precision-recip-sqrt
> > @opindex mno-low-precision-recip-sqrt
> >
> >
> >
> >
> > --
> > <rb18968.patch>
