[PATCH v2 2/2][ARM] Improve max_cond_insns setting for Cortex cores

[PATCH v2 2/2][ARM] Improve max_cond_insns setting for Cortex cores

[Wilco Dijkstra]

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.
--

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.

Re: [PATCH v2 2/2][ARM] Improve max_cond_insns setting for Cortex cores

[Kyrill Tkachov]

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.

Re: [PATCH v2 2/2][ARM] Improve max_cond_insns setting for Cortex cores

[Christophe Lyon]

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.

Re: [PATCH v2 2/2][ARM] Improve max_cond_insns setting for Cortex cores

[Wilco Dijkstra]

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

Re: [PATCH v2 2/2][ARM] Improve max_cond_insns setting for Cortex cores

[Christophe Lyon]

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

Re: [PATCH v2 2/2][ARM] Improve max_cond_insns setting for Cortex cores

[Wilco Dijkstra]

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

Re: [PATCH v2 2/2][ARM] Improve max_cond_insns setting for Cortex cores

[Christophe Lyon]

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

Re: [PATCH v2 2/2][ARM] Improve max_cond_insns setting for Cortex cores

[Wilco Dijkstra]

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

Re: [PATCH v2 2/2][ARM] Improve max_cond_insns setting for Cortex cores

[Christophe Lyon]

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

Re: [PATCH v2 2/2][ARM] Improve max_cond_insns setting for Cortex cores

[Wilco Dijkstra]

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