[v2,ARM] Disable code hoisting with -O3 (PR80155)

Message ID HE1PR0801MB2121DBD899AC0F5B4FA5A41083450@HE1PR0801MB2121.eurprd08.prod.outlook.com
State New
Headers show
Series
  • [v2,ARM] Disable code hoisting with -O3 (PR80155)
Related show

Commit Message

Wilco Dijkstra Nov. 26, 2019, 3:17 p.m.
Hi,

While code hoisting generally improves codesize, it can affect performance
negatively. Benchmarking shows it doesn't help SPEC and negatively affects
embedded benchmarks. Since the impact is relatively small with -O2 and mainly
affects -O3, the simplest option is to disable code hoisting for -O3 and higher.

OK for commit?

ChangeLog:
2019-11-26  Wilco Dijkstra  <wdijkstr@arm.com>

	PR tree-optimization/80155
	* common/config/arm/arm-common.c (arm_option_optimization_table):
	Disable -fcode-hoisting with -O3.
--

Comments

Christophe Lyon Nov. 26, 2019, 3:39 p.m. | #1
On Tue, 26 Nov 2019 at 16:18, Wilco Dijkstra <Wilco.Dijkstra@arm.com> wrote:
>

> Hi,

>

> While code hoisting generally improves codesize, it can affect performance

> negatively. Benchmarking shows it doesn't help SPEC and negatively affects

> embedded benchmarks. Since the impact is relatively small with -O2 and mainly

> affects -O3, the simplest option is to disable code hoisting for -O3 and higher.

>

> OK for commit?

>


Hi,

Some time ago, you proposed to enable code hoisting for -Os instead,
and this is the approach that was chosen
in arm-9-branch. Why are you proposing a different setting for trunk?

Thanks,

Christophe

> ChangeLog:

> 2019-11-26  Wilco Dijkstra  <wdijkstr@arm.com>

>

>         PR tree-optimization/80155

>         * common/config/arm/arm-common.c (arm_option_optimization_table):

>         Disable -fcode-hoisting with -O3.

> --

>

> diff --git a/gcc/common/config/arm/arm-common.c b/gcc/common/config/arm/arm-common.c

> index b761d3abd670a144a593c4b410b1e7fbdcb52f56..3e11f21b7dd76cc071b645c32a6fdb4a92511279 100644

> --- a/gcc/common/config/arm/arm-common.c

> +++ b/gcc/common/config/arm/arm-common.c

> @@ -39,6 +39,8 @@ static const struct default_options arm_option_optimization_table[] =

>      /* Enable section anchors by default at -O1 or higher.  */

>      { OPT_LEVELS_1_PLUS, OPT_fsection_anchors, NULL, 1 },

>      { OPT_LEVELS_FAST, OPT_fsched_pressure, NULL, 1 },

> +    /* Disable code hoisting with -O3 or higher.  */

> +    { OPT_LEVELS_3_PLUS, OPT_fcode_hoisting, NULL, 0 },

>      { OPT_LEVELS_NONE, 0, NULL, 0 }

>    };

>
Richard Biener Nov. 26, 2019, 3:43 p.m. | #2
On November 26, 2019 4:39:09 PM GMT+01:00, Christophe Lyon <christophe.lyon@linaro.org> wrote:
>On Tue, 26 Nov 2019 at 16:18, Wilco Dijkstra <Wilco.Dijkstra@arm.com>

>wrote:

>>

>> Hi,

>>

>> While code hoisting generally improves codesize, it can affect

>performance

>> negatively. Benchmarking shows it doesn't help SPEC and negatively

>affects

>> embedded benchmarks. Since the impact is relatively small with -O2

>and mainly

>> affects -O3, the simplest option is to disable code hoisting for -O3

>and higher.

>>

>> OK for commit?

>>

>

>Hi,

>

>Some time ago, you proposed to enable code hoisting for -Os instead,

>and this is the approach that was chosen

>in arm-9-branch. Why are you proposing a different setting for trunk?


These kind of tweaks also single out targets in intransparent ways for the user and for debugging code generation issues.

But it's your target... 

Richard 

>Thanks,

>

>Christophe

>

>> ChangeLog:

>> 2019-11-26  Wilco Dijkstra  <wdijkstr@arm.com>

>>

>>         PR tree-optimization/80155

>>         * common/config/arm/arm-common.c

>(arm_option_optimization_table):

>>         Disable -fcode-hoisting with -O3.

>> --

>>

>> diff --git a/gcc/common/config/arm/arm-common.c

>b/gcc/common/config/arm/arm-common.c

>> index

>b761d3abd670a144a593c4b410b1e7fbdcb52f56..3e11f21b7dd76cc071b645c32a6fdb4a92511279

>100644

>> --- a/gcc/common/config/arm/arm-common.c

>> +++ b/gcc/common/config/arm/arm-common.c

>> @@ -39,6 +39,8 @@ static const struct default_options

>arm_option_optimization_table[] =

>>      /* Enable section anchors by default at -O1 or higher.  */

>>      { OPT_LEVELS_1_PLUS, OPT_fsection_anchors, NULL, 1 },

>>      { OPT_LEVELS_FAST, OPT_fsched_pressure, NULL, 1 },

>> +    /* Disable code hoisting with -O3 or higher.  */

>> +    { OPT_LEVELS_3_PLUS, OPT_fcode_hoisting, NULL, 0 },

>>      { OPT_LEVELS_NONE, 0, NULL, 0 }

>>    };

>>
Wilco Dijkstra Nov. 26, 2019, 5:24 p.m. | #3
Hi Christophe,

> Some time ago, you proposed to enable code hoisting for -Os instead,

> and this is the approach that was chosen

> in arm-9-branch. Why are you proposing a different setting for trunk?


Like I said in my message, I've now done more detailed benchmarking which
shows it affects -O3 performance very significantly but -O2 by not much.
Since it's a good idea to keep the O1/O2 configs as standard as possible (as
Richi suggests), just disabling with -O3 seems better than the previous version.

Cheers,
Wilco
Wilco Dijkstra Jan. 21, 2020, 2:10 p.m. | #4
ping

Hi,

While code hoisting generally improves codesize, it can affect performance
negatively. Benchmarking shows it doesn't help SPEC and negatively affects
embedded benchmarks. Since the impact is relatively small with -O2 and mainly
affects -O3, the simplest option is to disable code hoisting for -O3 and higher.

OK for commit?

ChangeLog:
2019-11-26  Wilco Dijkstra  <wdijkstr@arm.com>

        PR tree-optimization/80155
        * common/config/arm/arm-common.c (arm_option_optimization_table):
        Disable -fcode-hoisting with -O3.
--

diff --git a/gcc/common/config/arm/arm-common.c b/gcc/common/config/arm/arm-common.c
index b761d3abd670a144a593c4b410b1e7fbdcb52f56..3e11f21b7dd76cc071b645c32a6fdb4a92511279 100644
--- a/gcc/common/config/arm/arm-common.c
+++ b/gcc/common/config/arm/arm-common.c
@@ -39,6 +39,8 @@ static const struct default_options arm_option_optimization_table[] =
     /* Enable section anchors by default at -O1 or higher.  */
     { OPT_LEVELS_1_PLUS, OPT_fsection_anchors, NULL, 1 },
     { OPT_LEVELS_FAST, OPT_fsched_pressure, NULL, 1 },
+    /* Disable code hoisting with -O3 or higher.  */
+    { OPT_LEVELS_3_PLUS, OPT_fcode_hoisting, NULL, 0 },
     { OPT_LEVELS_NONE, 0, NULL, 0 }
   };
Segher Boessenkool Jan. 27, 2020, 9:47 p.m. | #5
Hi!

On Tue, Jan 21, 2020 at 02:10:21PM +0000, Wilco Dijkstra wrote:
> While code hoisting generally improves codesize, it can affect performance

> negatively. Benchmarking shows it doesn't help SPEC and negatively affects

> embedded benchmarks. Since the impact is relatively small with -O2 and mainly

> affects -O3, the simplest option is to disable code hoisting for -O3 and higher.


Should this be a generic thing, not target-specific?


Segher
Richard Biener Jan. 28, 2020, 9:42 a.m. | #6
On Mon, Jan 27, 2020 at 10:47 PM Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>

> Hi!

>

> On Tue, Jan 21, 2020 at 02:10:21PM +0000, Wilco Dijkstra wrote:

> > While code hoisting generally improves codesize, it can affect performance

> > negatively. Benchmarking shows it doesn't help SPEC and negatively affects

> > embedded benchmarks. Since the impact is relatively small with -O2 and mainly

> > affects -O3, the simplest option is to disable code hoisting for -O3 and higher.

>

> Should this be a generic thing, not target-specific?


The change doesn't make sense - I'm sure if you'd look at the actual cases
it's not code hoisting in itself but "optimizations" enabled by it that cause
the issues.  It's probably the same thing as PRE causing issues downstream
for some benchmarks but do you disable PRE then by default just because of that?

Richard.

>

> Segher
Ramana Radhakrishnan Jan. 28, 2020, 11:53 a.m. | #7
On Tue, Nov 26, 2019 at 3:18 PM Wilco Dijkstra <Wilco.Dijkstra@arm.com> wrote:
>

> Hi,

>

> While code hoisting generally improves codesize, it can affect performance

> negatively. Benchmarking shows it doesn't help SPEC and negatively affects

> embedded benchmarks. Since the impact is relatively small with -O2 and mainly

> affects -O3, the simplest option is to disable code hoisting for -O3 and higher.

>

> OK for commit?

>

> ChangeLog:

> 2019-11-26  Wilco Dijkstra  <wdijkstr@arm.com>

>

>         PR tree-optimization/80155

>         * common/config/arm/arm-common.c (arm_option_optimization_table):

>         Disable -fcode-hoisting with -O3.

> --

>

> diff --git a/gcc/common/config/arm/arm-common.c b/gcc/common/config/arm/arm-common.c

> index b761d3abd670a144a593c4b410b1e7fbdcb52f56..3e11f21b7dd76cc071b645c32a6fdb4a92511279 100644

> --- a/gcc/common/config/arm/arm-common.c

> +++ b/gcc/common/config/arm/arm-common.c

> @@ -39,6 +39,8 @@ static const struct default_options arm_option_optimization_table[] =

>      /* Enable section anchors by default at -O1 or higher.  */

>      { OPT_LEVELS_1_PLUS, OPT_fsection_anchors, NULL, 1 },

>      { OPT_LEVELS_FAST, OPT_fsched_pressure, NULL, 1 },

> +    /* Disable code hoisting with -O3 or higher.  */

> +    { OPT_LEVELS_3_PLUS, OPT_fcode_hoisting, NULL, 0 },

>      { OPT_LEVELS_NONE, 0, NULL, 0 }

>    };

>


What are the cases in O3 where this is slower with embedded benchmarks
and how can we fix it ? Keeping the target "different" in this manner
doesn't augur well for the long term.

Ramana
Segher Boessenkool Jan. 28, 2020, 1:21 p.m. | #8
On Tue, Jan 28, 2020 at 10:42:16AM +0100, Richard Biener wrote:
> On Mon, Jan 27, 2020 at 10:47 PM Segher Boessenkool

> <segher@kernel.crashing.org> wrote:

> > On Tue, Jan 21, 2020 at 02:10:21PM +0000, Wilco Dijkstra wrote:

> > > While code hoisting generally improves codesize, it can affect performance

> > > negatively. Benchmarking shows it doesn't help SPEC and negatively affects

> > > embedded benchmarks. Since the impact is relatively small with -O2 and mainly

> > > affects -O3, the simplest option is to disable code hoisting for -O3 and higher.

> >

> > Should this be a generic thing, not target-specific?

> 

> The change doesn't make sense - I'm sure if you'd look at the actual cases

> it's not code hoisting in itself but "optimizations" enabled by it that cause

> the issues.  It's probably the same thing as PRE causing issues downstream

> for some benchmarks but do you disable PRE then by default just because of that?


Well, that depends on how bad it is.  And of course not if it is just
because of benchmark peculiarities, we're not a banchmark compiler after
all, we're meant to compile real code.  But if PRE (just to continue
your example) would mess that up more often than not, then yes, it
should not be enabled by default.


Segher

Patch

diff --git a/gcc/common/config/arm/arm-common.c b/gcc/common/config/arm/arm-common.c
index b761d3abd670a144a593c4b410b1e7fbdcb52f56..3e11f21b7dd76cc071b645c32a6fdb4a92511279 100644
--- a/gcc/common/config/arm/arm-common.c
+++ b/gcc/common/config/arm/arm-common.c
@@ -39,6 +39,8 @@  static const struct default_options arm_option_optimization_table[] =
     /* Enable section anchors by default at -O1 or higher.  */
     { OPT_LEVELS_1_PLUS, OPT_fsection_anchors, NULL, 1 },
     { OPT_LEVELS_FAST, OPT_fsched_pressure, NULL, 1 },
+    /* Disable code hoisting with -O3 or higher.  */
+    { OPT_LEVELS_3_PLUS, OPT_fcode_hoisting, NULL, 0 },
     { OPT_LEVELS_NONE, 0, NULL, 0 }
   };