[S390] Increase function alignment to 16 bytes

Message ID 3a423288-3685-1231-697e-0f51236a1726@linux.ibm.com
State New
Headers show
Series
  • [S390] Increase function alignment to 16 bytes
Related show

Commit Message

Robin Dapp July 11, 2018, 3:40 p.m.
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.

Comments

Martin Liška July 12, 2018, 10:21 a.m. | #1
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 },
Robin Dapp July 12, 2018, 11:34 a.m. | #2
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);
+}
Andreas Krebbel July 13, 2018, 8:14 a.m. | #3
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

Patch

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