| Message ID | 3a423288-3685-1231-697e-0f51236a1726@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [S390] Increase function alignment to 16 bytes | expand |
On 07/11/2018 05:40 PM, Robin Dapp wrote: > Hi, > > the following patch increases the default function alignment to 16 > bytes. This helps get rid of some unwanted performance effects. > > I'm unsure whether or when it's necessary to implement > OVERRIDE_OPTIONS_AFTER_CHANGE. Hi. Yes, it's how that should be done. That allows GCC to properly work with per-function attributes (or corresponding #pragma). > Apparently ia64 did it to set flags that are reset when using > __attribute__((optimize)). i386 calls i386_default_align () and sets > various alignments only when the alignment value is unset but when is > e.g. global_options.x_str_align_functions actually unset except for the > very first call? That said, it's probably wrongly implemented in ia64. But as it's legacy stuff, I'm probably not planning to fix it. > > Trying simple examples like > > > void foo () {}; > > __attribute__((optimize("Os"))) > void bar () {}; > > > I did not observe that the default alignment, once set, was reset anywhere. That should be set in TARGET_OVERRIDE_OPTIONS_AFTER_CHANGE. Please skip '+ && !opts->x_optimize_size)'. I'm attaching patch that will set opts->x_flag_align_functions to 0 for -Os. It's part of another batch alignment patches I'm preparing. Note that even when an alignment is set, we have optimize_function_for_speed_p (cfun)) guards in assembly emission that do not produce any alignment. Martin > > Regards > Robin > > -- > > gcc/ChangeLog: > > 2018-07-11 Robin Dapp <rdapp@linux.ibm.com> > > * config/s390/s390.c (s390_default_align): Set default > function alignment. > (s390_override_options_after_change): New. > (s390_option_override_internal): Call s390_default_align. > (TARGET_OVERRIDE_OPTIONS_AFTER_CHANGE): New. > From ee6832050321149f9c0d180835658a684136ef58 Mon Sep 17 00:00:00 2001 From: marxin <mliska@suse.cz> Date: Thu, 12 Jul 2018 12:08:09 +0200 Subject: [PATCH] Do not enable OPT_falign_* for -Os. gcc/ChangeLog: 2018-07-12 Martin Liska <mliska@suse.cz> * opts.c: Do not enable OPT_falign_* for -Os. --- gcc/opts.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/gcc/opts.c b/gcc/opts.c index 0625b15b27b..b8ae8756b4f 100644 --- a/gcc/opts.c +++ b/gcc/opts.c @@ -510,10 +510,10 @@ static const struct default_options default_options_table[] = { OPT_LEVELS_2_PLUS, OPT_fdevirtualize, NULL, 1 }, { OPT_LEVELS_2_PLUS, OPT_fdevirtualize_speculatively, NULL, 1 }, { OPT_LEVELS_2_PLUS, OPT_fipa_sra, NULL, 1 }, - { OPT_LEVELS_2_PLUS, OPT_falign_loops, NULL, 1 }, - { OPT_LEVELS_2_PLUS, OPT_falign_jumps, NULL, 1 }, - { OPT_LEVELS_2_PLUS, OPT_falign_labels, NULL, 1 }, - { OPT_LEVELS_2_PLUS, OPT_falign_functions, NULL, 1 }, + { OPT_LEVELS_2_PLUS_SPEED_ONLY, OPT_falign_loops, NULL, 1 }, + { OPT_LEVELS_2_PLUS_SPEED_ONLY, OPT_falign_jumps, NULL, 1 }, + { OPT_LEVELS_2_PLUS_SPEED_ONLY, OPT_falign_labels, NULL, 1 }, + { OPT_LEVELS_2_PLUS_SPEED_ONLY, OPT_falign_functions, NULL, 1 }, { OPT_LEVELS_2_PLUS, OPT_ftree_tail_merge, NULL, 1 }, { OPT_LEVELS_2_PLUS, OPT_fvect_cost_model_, NULL, VECT_COST_MODEL_CHEAP }, { OPT_LEVELS_2_PLUS_SPEED_ONLY, OPT_foptimize_strlen, NULL, 1 },
Hi, > Please skip '+ && !opts->x_optimize_size)'. I'm attaching patch > that will > set opts->x_flag_align_functions to 0 for -Os. It's part of another batch > alignment patches I'm preparing. done in the attached version and added some tests (which do not all fail without the patch as we can get lucky with the alignment). Regtested on s390x. Regards Robin -- gcc/ChangeLog: 2018-07-12 Robin Dapp <rdapp@linux.ibm.com> * config/s390/s390.c (s390_default_align): Set default function alignment to 16. (s390_override_options_after_change): Call s390_default align. (s390_option_override_internal): Call s390_default_align. (TARGET_OVERRIDE_OPTIONS_AFTER_CHANGE): Define. gcc/testsuite/ChangeLog: 2018-07-12 Robin Dapp <rdapp@linux.ibm.com> * gcc.target/s390/function-align1.c: New test. * gcc.target/s390/function-align2.c: New test. * gcc.target/s390/function-align3.c: New test. diff --git a/gcc/config/s390/s390.c b/gcc/config/s390/s390.c index 8df195ddd78..af4cc702259 100644 --- a/gcc/config/s390/s390.c +++ b/gcc/config/s390/s390.c @@ -15322,6 +15322,22 @@ s390_function_specific_restore (struct gcc_options *opts, opts->x_s390_cost_pointer = (long)processor_table[opts->x_s390_tune].cost; } +static void +s390_default_align (struct gcc_options *opts) +{ + /* Set the default function alignment to 16 in order to get rid of + some unwanted performance effects. */ + if (opts->x_flag_align_functions && !opts->x_str_align_functions + && opts->x_s390_tune >= PROCESSOR_2964_Z13) + opts->x_str_align_functions = "16"; +} + +static void +s390_override_options_after_change (void) +{ + s390_default_align (&global_options); +} + static void s390_option_override_internal (bool main_args_p, struct gcc_options *opts, @@ -15559,6 +15575,9 @@ s390_option_override_internal (bool main_args_p, opts->x_param_values, opts_set->x_param_values); + /* Set the default alignment. */ + s390_default_align (opts); + /* Call target specific restore function to do post-init work. At the moment, this just sets opts->x_s390_cost_pointer. */ s390_function_specific_restore (opts, NULL); @@ -16751,6 +16770,9 @@ s390_case_values_threshold (void) #undef TARGET_PASS_BY_REFERENCE #define TARGET_PASS_BY_REFERENCE s390_pass_by_reference +#undef TARGET_OVERRIDE_OPTIONS_AFTER_CHANGE +#define TARGET_OVERRIDE_OPTIONS_AFTER_CHANGE s390_override_options_after_change + #undef TARGET_FUNCTION_OK_FOR_SIBCALL #define TARGET_FUNCTION_OK_FOR_SIBCALL s390_function_ok_for_sibcall #undef TARGET_FUNCTION_ARG diff --git a/gcc/testsuite/gcc.target/s390/function-align1.c b/gcc/testsuite/gcc.target/s390/function-align1.c new file mode 100644 index 00000000000..78fa563098e --- /dev/null +++ b/gcc/testsuite/gcc.target/s390/function-align1.c @@ -0,0 +1,24 @@ +/* { dg-do run } */ +/* { dg-options "-O2 -march=z13" } */ + +#include <assert.h> +#include <stdint.h> + +__attribute__((noinline)) +void foo1 () {} + +__attribute__((noinline)) +__attribute__((optimize("align-functions=32"))) +void foo2 () {} + +int main () +{ + foo1 (); + foo2 (); + + void *f = &foo1; + void *g = &foo2; + + assert (((uintptr_t)f % 16) == 0); + assert (((uintptr_t)g % 32) == 0); +} diff --git a/gcc/testsuite/gcc.target/s390/function-align2.c b/gcc/testsuite/gcc.target/s390/function-align2.c new file mode 100644 index 00000000000..0d8e1ff2251 --- /dev/null +++ b/gcc/testsuite/gcc.target/s390/function-align2.c @@ -0,0 +1,13 @@ +/* { dg-do compile } */ +/* { dg-options "-Os -march=z13" } */ + +void bar () +{ + /* { dg-final { scan-assembler-times ".align\t8" 2 } } */ +} + +__attribute__((optimize("O2"))) +void baz () +{ + /* { dg-final { scan-assembler-times ".align\t16" 1 } } */ +} diff --git a/gcc/testsuite/gcc.target/s390/function-align3.c b/gcc/testsuite/gcc.target/s390/function-align3.c new file mode 100644 index 00000000000..adb79763d4a --- /dev/null +++ b/gcc/testsuite/gcc.target/s390/function-align3.c @@ -0,0 +1,22 @@ +/* { dg-do run } */ +/* { dg-options "-Os -march=z13" } */ + +#include <assert.h> +#include <stdint.h> + +__attribute__((noinline)) +void bar () {} + +__attribute__((noinline)) +__attribute__((optimize("O2"))) +void baf () {} + +int main () +{ + bar (); + baf (); + + void *g = &baf; + + assert ( ((uintptr_t)g % 16) == 0); +}
On 07/12/2018 01:34 PM, Robin Dapp wrote: > Hi, > >> Please skip '+ && !opts->x_optimize_size)'. I'm attaching patch >> that will >> set opts->x_flag_align_functions to 0 for -Os. It's part of another batch >> alignment patches I'm preparing. > > done in the attached version and added some tests (which do not all fail > without the patch as we can get lucky with the alignment). > > Regtested on s390x. > > Regards > Robin > > -- > > gcc/ChangeLog: > > 2018-07-12 Robin Dapp <rdapp@linux.ibm.com> > > * config/s390/s390.c (s390_default_align): Set default function > alignment to 16. > (s390_override_options_after_change): Call s390_default align. > (s390_option_override_internal): Call s390_default_align. > (TARGET_OVERRIDE_OPTIONS_AFTER_CHANGE): Define. > > gcc/testsuite/ChangeLog: > > 2018-07-12 Robin Dapp <rdapp@linux.ibm.com> > > * gcc.target/s390/function-align1.c: New test. > * gcc.target/s390/function-align2.c: New test. > * gcc.target/s390/function-align3.c: New test. > Ok to apply. Thanks! Andreas
diff --git a/gcc/config/s390/s390.c b/gcc/config/s390/s390.c index 8df195ddd78..eaeba89b321 100644 --- a/gcc/config/s390/s390.c +++ b/gcc/config/s390/s390.c @@ -15322,6 +15322,23 @@ s390_function_specific_restore (struct gcc_options *opts, opts->x_s390_cost_pointer = (long)processor_table[opts->x_s390_tune].cost; } +static void +s390_default_align (struct gcc_options *opts) +{ + /* Set the default function alignment to 16 in order to get rid of + some unwanted performance effects. */ + if (opts->x_flag_align_functions && !opts->x_str_align_functions + && opts->x_s390_tune >= PROCESSOR_2964_Z13 + && !opts->x_optimize_size) + opts->x_str_align_functions = "16"; +} + +static void +s390_override_options_after_change (void) +{ + s390_default_align (&global_options); +} + static void s390_option_override_internal (bool main_args_p, struct gcc_options *opts, @@ -15559,6 +15576,9 @@ s390_option_override_internal (bool main_args_p, opts->x_param_values, opts_set->x_param_values); + /* Set the default alignment. */ + s390_default_align (opts); + /* Call target specific restore function to do post-init work. At the moment, this just sets opts->x_s390_cost_pointer. */ s390_function_specific_restore (opts, NULL); @@ -16751,6 +16771,9 @@ s390_case_values_threshold (void) #undef TARGET_PASS_BY_REFERENCE #define TARGET_PASS_BY_REFERENCE s390_pass_by_reference +#undef TARGET_OVERRIDE_OPTIONS_AFTER_CHANGE +#define TARGET_OVERRIDE_OPTIONS_AFTER_CHANGE s390_override_options_after_change + #undef TARGET_FUNCTION_OK_FOR_SIBCALL #define TARGET_FUNCTION_OK_FOR_SIBCALL s390_function_ok_for_sibcall #undef TARGET_FUNCTION_ARG
Hi, the following patch increases the default function alignment to 16 bytes. This helps get rid of some unwanted performance effects. I'm unsure whether or when it's necessary to implement OVERRIDE_OPTIONS_AFTER_CHANGE. Apparently ia64 did it to set flags that are reset when using __attribute__((optimize)). i386 calls i386_default_align () and sets various alignments only when the alignment value is unset but when is e.g. global_options.x_str_align_functions actually unset except for the very first call? Trying simple examples like void foo () {}; __attribute__((optimize("Os"))) void bar () {}; I did not observe that the default alignment, once set, was reset anywhere. Regards Robin -- gcc/ChangeLog: 2018-07-11 Robin Dapp <rdapp@linux.ibm.com> * config/s390/s390.c (s390_default_align): Set default function alignment. (s390_override_options_after_change): New. (s390_option_override_internal): Call s390_default_align. (TARGET_OVERRIDE_OPTIONS_AFTER_CHANGE): New.