| Message ID | VI1PR0801MB212760880CC4ACDC14FCC52683420@VI1PR0801MB2127.eurprd08.prod.outlook.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On 12/3/19 1:45 PM, Wilco Dijkstra wrote: > Hi, > > Part 2, split off from > https://gcc.gnu.org/ml/gcc-patches/2019-11/msg00399.html > > To enable cores to use the correct max_cond_insns setting, use the > core-specific > tuning when a CPU/tune is selected unless -mrestrict-it is explicitly set. > > On Cortex-A57 this gives 1.1% performance gain on SPECINT2006 as well as a > 0.4% codesize reduction. > > Bootstrapped on armhf. OK for commit? > Ok. Thanks, Kyrill > ChangeLog: > > 2019-12-03 Wilco Dijkstra <wdijkstr@arm.com> > > * config/arm/arm.c (arm_option_override_internal): > Use max_cond_insns from CPU tuning unless -mrestrict-it is used. > -- > > diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c > index > daebe76352d62ad94556762b4e3bc3d0532ad411..5ed9046988996e56f754c5588e4d25d5ecdd6b03 > 100644 > --- a/gcc/config/arm/arm.c > +++ b/gcc/config/arm/arm.c > @@ -3041,6 +3041,11 @@ arm_option_override_internal (struct > gcc_options *opts, > if (!TARGET_THUMB2_P (opts->x_target_flags) || !arm_arch_notm) > opts->x_arm_restrict_it = 0; > > + /* Use the IT size from CPU specific tuning unless -mrestrict-it is > used. */ > + if (!opts_set->x_arm_restrict_it > + && (opts_set->x_arm_cpu_string || opts_set->x_arm_tune_string)) > + opts->x_arm_restrict_it = 0; > + > /* Enable -munaligned-access by default for > - all ARMv6 architecture-based processors when compiling for a > 32-bit ISA > i.e. Thumb2 and ARM state only.
On Tue, 3 Dec 2019 at 14:45, Wilco Dijkstra <Wilco.Dijkstra@arm.com> wrote: > > Hi, > > Part 2, split off from https://gcc.gnu.org/ml/gcc-patches/2019-11/msg00399.html > > To enable cores to use the correct max_cond_insns setting, use the core-specific > tuning when a CPU/tune is selected unless -mrestrict-it is explicitly set. > Hi, This patch (r278968) is causing regressions when building GCC --target arm-none-linux-gnueabihf --with-mode thumb --with-cpu cortex-a57 --with-fpu crypto-neon-fp-armv8 because the assembler (gas version 2.33.1) complains: /ccc7z5eW.s:4267: IT blocks containing more than one conditional instruction are performance deprecated in ARMv8-A and ARMv8-R I guess that's related to what you say about -mrestrict-it ? Christophe > On Cortex-A57 this gives 1.1% performance gain on SPECINT2006 as well as a > 0.4% codesize reduction. > > Bootstrapped on armhf. OK for commit? > > ChangeLog: > > 2019-12-03 Wilco Dijkstra <wdijkstr@arm.com> > > * config/arm/arm.c (arm_option_override_internal): > Use max_cond_insns from CPU tuning unless -mrestrict-it is used. > -- > > diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c > index daebe76352d62ad94556762b4e3bc3d0532ad411..5ed9046988996e56f754c5588e4d25d5ecdd6b03 100644 > --- a/gcc/config/arm/arm.c > +++ b/gcc/config/arm/arm.c > @@ -3041,6 +3041,11 @@ arm_option_override_internal (struct gcc_options *opts, > if (!TARGET_THUMB2_P (opts->x_target_flags) || !arm_arch_notm) > opts->x_arm_restrict_it = 0; > > + /* Use the IT size from CPU specific tuning unless -mrestrict-it is used. */ > + if (!opts_set->x_arm_restrict_it > + && (opts_set->x_arm_cpu_string || opts_set->x_arm_tune_string)) > + opts->x_arm_restrict_it = 0; > + > /* Enable -munaligned-access by default for > - all ARMv6 architecture-based processors when compiling for a 32-bit ISA > i.e. Thumb2 and ARM state only.
Hi Christophe, > This patch (r278968) is causing regressions when building GCC > --target arm-none-linux-gnueabihf > --with-mode thumb > --with-cpu cortex-a57 > --with-fpu crypto-neon-fp-armv8 > because the assembler (gas version 2.33.1) complains: > /ccc7z5eW.s:4267: IT blocks containing more than one conditional > instruction are performance deprecated in ARMv8-A and ARMv8-R > > I guess that's related to what you say about -mrestrict-it ? Yes it looks like that unnecessary warning hasn't been silenced in latest binutils, but it should be easy to turn off. Cheers, Wilco
On Fri, 6 Dec 2019 at 11:47, Wilco Dijkstra <Wilco.Dijkstra@arm.com> wrote: > > Hi Christophe, > > > This patch (r278968) is causing regressions when building GCC > > --target arm-none-linux-gnueabihf > > --with-mode thumb > > --with-cpu cortex-a57 > > --with-fpu crypto-neon-fp-armv8 > > because the assembler (gas version 2.33.1) complains: > > /ccc7z5eW.s:4267: IT blocks containing more than one conditional > > instruction are performance deprecated in ARMv8-A and ARMv8-R > > > > I guess that's related to what you say about -mrestrict-it ? > > Yes it looks like that unnecessary warning hasn't been silenced in latest binutils, > but it should be easy to turn off. > Do you mean that binutils should be "fixed" not to emit this warning (since you say it's unnecessary), or GCC made not to emit the offending sequence? > Cheers, > Wilco
Hi Christophe, I've added an option to allow the warning to be enabled/disabled: https://sourceware.org/ml/binutils/2019-12/msg00093.html Cheers, Wilco
On Fri, 6 Dec 2019 at 16:03, Wilco Dijkstra <Wilco.Dijkstra@arm.com> wrote: > > Hi Christophe, > > I've added an option to allow the warning to be enabled/disabled: > https://sourceware.org/ml/binutils/2019-12/msg00093.html > Thanks. In practice, how do you activate it when running the GCC testsuite? Do you plan to send a GCC patch to enable this assembler flag, or do you locally enable that option by default in your binutils? FWIW, I've also noticed that the whole libstdc++ testsuite is somehow "deactivated" (I have 0 pass, 0 fail etc...) after your GCC patch when configuring GCC --target arm-none-linux-gnueabihf --with-mode thumb --with-cpu cortex-a57 --with-fpu crypto-neon-fp-armv8 Or do you filter these warnings in dejagnu ? > Cheers, > Wilco
Hi Christophe, > In practice, how do you activate it when running the GCC testsuite? Do > you plan to send a GCC patch to enable this assembler flag, or do you > locally enable that option by default in your binutils? The warning is off by default so there is no need to do anything in the testsuite, you just need a fixed binutils. > FWIW, I've also noticed that the whole libstdc++ testsuite is somehow > "deactivated" (I have 0 pass, 0 fail etc...) after your GCC patch > when configuring GCC > --target arm-none-linux-gnueabihf > --with-mode thumb > --with-cpu cortex-a57 > --with-fpu crypto-neon-fp-armv8 Well it's possible a configure check failed somehow. Cheers, Wilco
On Fri, 6 Dec 2019 at 19:47, Wilco Dijkstra <Wilco.Dijkstra@arm.com> wrote: > > Hi Christophe, > > > In practice, how do you activate it when running the GCC testsuite? Do > > you plan to send a GCC patch to enable this assembler flag, or do you > > locally enable that option by default in your binutils? > > The warning is off by default so there is no need to do anything in the testsuite, > you just need a fixed binutils. > Don't we want to fix GCC to stop generating the offending sequence? > > FWIW, I've also noticed that the whole libstdc++ testsuite is somehow > > "deactivated" (I have 0 pass, 0 fail etc...) after your GCC patch > > when configuring GCC > > --target arm-none-linux-gnueabihf > > --with-mode thumb > > --with-cpu cortex-a57 > > --with-fpu crypto-neon-fp-armv8 > > Well it's possible a configure check failed somehow. > Yes, it fails when compiling testsuite_abi.cc, resulting in tcl errors. > Cheers, > Wilco
Hi Christophe, >> The warning is off by default so there is no need to do anything in the testsuite, >> you just need a fixed binutils. >> > > Don't we want to fix GCC to stop generating the offending sequence? Why? All ARMv8 implementations have to support it, and despite the warning code actually runs significantly faster. >> Well it's possible a configure check failed somehow. >> > Yes, it fails when compiling testsuite_abi.cc, resulting in tcl errors. It's odd it's that sensitive to extra warnings, but anyway... Cheers, Wilco
diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c index daebe76352d62ad94556762b4e3bc3d0532ad411..5ed9046988996e56f754c5588e4d25d5ecdd6b03 100644 --- a/gcc/config/arm/arm.c +++ b/gcc/config/arm/arm.c @@ -3041,6 +3041,11 @@ arm_option_override_internal (struct gcc_options *opts, if (!TARGET_THUMB2_P (opts->x_target_flags) || !arm_arch_notm) opts->x_arm_restrict_it = 0; + /* Use the IT size from CPU specific tuning unless -mrestrict-it is used. */ + if (!opts_set->x_arm_restrict_it + && (opts_set->x_arm_cpu_string || opts_set->x_arm_tune_string)) + opts->x_arm_restrict_it = 0; + /* Enable -munaligned-access by default for - all ARMv6 architecture-based processors when compiling for a 32-bit ISA i.e. Thumb2 and ARM state only.
Hi, Part 2, split off from https://gcc.gnu.org/ml/gcc-patches/2019-11/msg00399.html To enable cores to use the correct max_cond_insns setting, use the core-specific tuning when a CPU/tune is selected unless -mrestrict-it is explicitly set. On Cortex-A57 this gives 1.1% performance gain on SPECINT2006 as well as a 0.4% codesize reduction. Bootstrapped on armhf. OK for commit? ChangeLog: 2019-12-03 Wilco Dijkstra <wdijkstr@arm.com> * config/arm/arm.c (arm_option_override_internal): Use max_cond_insns from CPU tuning unless -mrestrict-it is used. --