[PATCH, x86] Add TARGET_OVERRIDE_OPTIONS_AFTER_CHANGE hook

[PATCH, x86] Add TARGET_OVERRIDE_OPTIONS_AFTER_CHANGE hook

[Christian Bruel]

[-- Attachment #1: Type: text/plain, Size: 1815 bytes --]

While trying to reduce the PR64835 case for ARM and x86, I noticed that 
the alignment flags are cleared for x86 when attribute optimized is used.

With the attached testcases, the visible effects are twofold :

1) Functions compiled in with attribute optimize (-O2) are not aligned 
as if they were with the -O2 flag.

2) can_inline_edge_p fails because opts_for_fn (caller->decl) != 
opts_for_fn (callee->decl)) even-though they are compiled with the same 
optimization level.

indeed we have:

  <optimization_node 0x7ffff7522000
     align_functions (0x10)
     align_jumps (0x10)
     align_loops (0x10)
     flag_sched_stalled_insns_dep (0x1)
     flag_tree_parallelize_loops (0x1)
     flag_fp_contract_mode (0x2)
     flag_ira_algorithm (0)
     flag_ira_region (0x2)
     flag_simd_cost_model (0)
     flag_vect_cost_model (0x1)
     optimize (0x2)
     flag_aggressive_loop_optimizations (0x1)
     ...

<optimization_node 0x7ffff7522188
     flag_sched_stalled_insns_dep (0x1)
     flag_tree_parallelize_loops (0x1)
     flag_fp_contract_mode (0x2)
     flag_ira_algorithm (0)
     flag_ira_region (0x2)
     flag_simd_cost_model (0)
     flag_vect_cost_model (0x1)
     optimize (0x2)
     flag_aggressive_loop_optimizations (0x1)
     ...

The problem is that the alignment flags are not recomputed when setting 
the attribute flags in DECL_FUNCTION_SPECIFIC_OPTIMIZATION. Implementing 
the TARGET_OVERRIDE_OPTIONS_AFTER_CHANGE hook to set them fixes the problem.

NB: TARGET_OVERRIDE_OPTIONS_AFTER_CHANGE is also used to hold options 
from attribute target, so.this patch is a prerequisite to fix PR/64835 
on ARM and x86

bootstrapped, regtested with no new failures for x86_64-unknown-linux-gnu

Comments ? I'd like to candidate this for trunk when stage1 opens again.

Many Thanks

Christian







[-- Attachment #2: attr-align.patch --]
[-- Type: text/x-patch, Size: 4408 bytes --]

2015-02-06  Christian Bruel  <christian.bruel@st.com>

	PR target/64835
	* config/i386/i386.c (ix86_default_align): New function.
	(ix86_override_options_after_change): Call ix86_default_align.
	(TARGET_OVERRIDE_OPTIONS_AFTER_CHANGE): New hook.
	(ix86_override_options_after_change): New function.

2015-02-06  Christian Bruel  <christian.bruel@st.com>

	PR target/64835
	* gcc.dg/ipa/iinline-attr.c: New test.
	* gcc.target/i386/iinline-attr-2.c: New test.

Index: gcc/config/i386/i386.c
===================================================================
--- gcc/config/i386/i386.c	(revision 220394)
+++ gcc/config/i386/i386.c	(working copy)
@@ -3105,6 +3105,35 @@
 }
 
 
+/* Default align_* from the processor table.  */
+
+static void
+ix86_default_align (struct gcc_options *opts)
+{
+  if (opts->x_align_loops == 0)
+    {
+      opts->x_align_loops = processor_target_table[ix86_tune].align_loop;
+      align_loops_max_skip = processor_target_table[ix86_tune].align_loop_max_skip;
+    }
+  if (opts->x_align_jumps == 0)
+    {
+      opts->x_align_jumps = processor_target_table[ix86_tune].align_jump;
+      align_jumps_max_skip = processor_target_table[ix86_tune].align_jump_max_skip;
+    }
+  if (opts->x_align_functions == 0)
+    {
+      opts->x_align_functions = processor_target_table[ix86_tune].align_func;
+    }
+}
+
+/* Implement TARGET_OVERRIDE_OPTIONS_AFTER_CHANGE hook.  */
+
+static void
+ix86_override_options_after_change (void)
+{
+  ix86_default_align (&global_options);
+}
+
 /* Override various settings based on options.  If MAIN_ARGS_P, the
    options are from the command line, otherwise they are from
    attributes.  */
@@ -3902,20 +3931,7 @@
     opts->x_ix86_regparm = REGPARM_MAX;
 
   /* Default align_* from the processor table.  */
-  if (opts->x_align_loops == 0)
-    {
-      opts->x_align_loops = processor_target_table[ix86_tune].align_loop;
-      align_loops_max_skip = processor_target_table[ix86_tune].align_loop_max_skip;
-    }
-  if (opts->x_align_jumps == 0)
-    {
-      opts->x_align_jumps = processor_target_table[ix86_tune].align_jump;
-      align_jumps_max_skip = processor_target_table[ix86_tune].align_jump_max_skip;
-    }
-  if (opts->x_align_functions == 0)
-    {
-      opts->x_align_functions = processor_target_table[ix86_tune].align_func;
-    }
+  ix86_default_align (opts);
 
   /* Provide default for -mbranch-cost= value.  */
   if (!opts_set->x_ix86_branch_cost)
@@ -51928,6 +51944,9 @@
 #undef TARGET_PROMOTE_FUNCTION_MODE
 #define TARGET_PROMOTE_FUNCTION_MODE ix86_promote_function_mode
 
+#undef  TARGET_OVERRIDE_OPTIONS_AFTER_CHANGE
+#define TARGET_OVERRIDE_OPTIONS_AFTER_CHANGE ix86_override_options_after_change
+
 #undef TARGET_MEMBER_TYPE_FORCES_BLK
 #define TARGET_MEMBER_TYPE_FORCES_BLK ix86_member_type_forces_blk
 
Index: gcc/testsuite/gcc.dg/ipa/iinline-attr.c
===================================================================
--- gcc/testsuite/gcc.dg/ipa/iinline-attr.c	(revision 0)
+++ gcc/testsuite/gcc.dg/ipa/iinline-attr.c	(working copy)
@@ -0,0 +1,27 @@
+/* Verify that simple indirect calls are inlined even when
+   attribute __optimize is used.  */
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-ipa-inline"  } */
+
+extern void non_existent(int);
+
+static void hooray ()
+{
+  non_existent (1);
+}
+
+__attribute__ ((__optimize__ ("O2")))
+static void hiphip (void (*f)())
+{
+  non_existent (2);
+  f ();
+}
+
+int test (void)
+{
+  hiphip (hooray);
+  return 0;
+}
+
+/* { dg-final { scan-ipa-dump "hooray\[^\\n\]*inline copy in test"  "inline"  } } */
+/* { dg-final { cleanup-ipa-dump "inline" } } */
Index: gcc/testsuite/gcc.target/i386/iinline-attr-2.c
===================================================================
--- gcc/testsuite/gcc.target/i386/iinline-attr-2.c	(revision 0)
+++ gcc/testsuite/gcc.target/i386/iinline-attr-2.c	(working copy)
@@ -0,0 +1,29 @@
+/* Verify that alignment flags are set when  attribute __optimize is used.  */
+/* { dg-do compile } */
+/* { dg-options "-O2"  } */
+
+extern void non_existent(int);
+
+__attribute__ ((__optimize__ ("O2")))
+static void hooray ()
+{
+  non_existent (1);
+}
+
+__attribute__ ((__optimize__ ("O2")))
+static void hiphip (void (*f)())
+{
+  non_existent (2);
+  f ();
+}
+
+__attribute__ ((__optimize__ ("O2")))
+int test (void)
+{
+  hiphip (hooray);
+  return 0;
+}
+
+/* { dg-final { scan-assembler "p2align" } } */
+
+

Re: [PATCH, x86] Add TARGET_OVERRIDE_OPTIONS_AFTER_CHANGE hook

[Uros Bizjak]

On Wed, Feb 4, 2015 at 2:21 PM, Christian Bruel <christian.bruel@st.com> wrote:
> While trying to reduce the PR64835 case for ARM and x86, I noticed that the
> alignment flags are cleared for x86 when attribute optimized is used.
>
> With the attached testcases, the visible effects are twofold :
>
> 1) Functions compiled in with attribute optimize (-O2) are not aligned as if
> they were with the -O2 flag.
>
> 2) can_inline_edge_p fails because opts_for_fn (caller->decl) != opts_for_fn
> (callee->decl)) even-though they are compiled with the same optimization
> level.

2015-02-06  Christian Bruel  <christian.bruel@st.com>

    PR target/64835
    * config/i386/i386.c (ix86_default_align): New function.
    (ix86_override_options_after_change): Call ix86_default_align.
    (TARGET_OVERRIDE_OPTIONS_AFTER_CHANGE): New hook.
    (ix86_override_options_after_change): New function.

2015-02-06  Christian Bruel  <christian.bruel@st.com>

    PR target/64835
    * gcc.dg/ipa/iinline-attr.c: New test.
    * gcc.target/i386/iinline-attr-2.c: New test.

OK for mainline.

Thanks,
Uros

Re: [PATCH, x86] Add TARGET_OVERRIDE_OPTIONS_AFTER_CHANGE hook

[Bin.Cheng]

On Mon, Apr 27, 2015 at 8:01 PM, Uros Bizjak <ubizjak@gmail.com> wrote:
> On Wed, Feb 4, 2015 at 2:21 PM, Christian Bruel <christian.bruel@st.com> wrote:
>> While trying to reduce the PR64835 case for ARM and x86, I noticed that the
>> alignment flags are cleared for x86 when attribute optimized is used.
>>
>> With the attached testcases, the visible effects are twofold :
>>
>> 1) Functions compiled in with attribute optimize (-O2) are not aligned as if
>> they were with the -O2 flag.
>>
>> 2) can_inline_edge_p fails because opts_for_fn (caller->decl) != opts_for_fn
>> (callee->decl)) even-though they are compiled with the same optimization
>> level.
>
> 2015-02-06  Christian Bruel  <christian.bruel@st.com>
>
>     PR target/64835
>     * config/i386/i386.c (ix86_default_align): New function.
>     (ix86_override_options_after_change): Call ix86_default_align.
>     (TARGET_OVERRIDE_OPTIONS_AFTER_CHANGE): New hook.
>     (ix86_override_options_after_change): New function.
>
> 2015-02-06  Christian Bruel  <christian.bruel@st.com>
>
>     PR target/64835
>     * gcc.dg/ipa/iinline-attr.c: New test.
>     * gcc.target/i386/iinline-attr-2.c: New test.
>
> OK for mainline.

Hi Christian,
I noticed case gcc.dg/ipa/iinline-attr.c failed on aarch64.  The
original patch is x86 specific, while the case is added as general
one.  Could you please have a look at this?

FAIL: gcc.dg/ipa/iinline-attr.c scan-ipa-dump inline
"hooray[^\\n]*inline copy in test"

Thanks,
bin
>
> Thanks,
> Uros

Re: [PATCH, x86] Add TARGET_OVERRIDE_OPTIONS_AFTER_CHANGE hook

[Christian Bruel]

OK I've have a look,

thanks

Christian


On 04/30/2015 10:27 AM, Bin.Cheng wrote:
> On Mon, Apr 27, 2015 at 8:01 PM, Uros Bizjak <ubizjak@gmail.com> wrote:
>> On Wed, Feb 4, 2015 at 2:21 PM, Christian Bruel <christian.bruel@st.com> wrote:
>>> While trying to reduce the PR64835 case for ARM and x86, I noticed that the
>>> alignment flags are cleared for x86 when attribute optimized is used.
>>>
>>> With the attached testcases, the visible effects are twofold :
>>>
>>> 1) Functions compiled in with attribute optimize (-O2) are not aligned as if
>>> they were with the -O2 flag.
>>>
>>> 2) can_inline_edge_p fails because opts_for_fn (caller->decl) != opts_for_fn
>>> (callee->decl)) even-though they are compiled with the same optimization
>>> level.
>>
>> 2015-02-06  Christian Bruel  <christian.bruel@st.com>
>>
>>     PR target/64835
>>     * config/i386/i386.c (ix86_default_align): New function.
>>     (ix86_override_options_after_change): Call ix86_default_align.
>>     (TARGET_OVERRIDE_OPTIONS_AFTER_CHANGE): New hook.
>>     (ix86_override_options_after_change): New function.
>>
>> 2015-02-06  Christian Bruel  <christian.bruel@st.com>
>>
>>     PR target/64835
>>     * gcc.dg/ipa/iinline-attr.c: New test.
>>     * gcc.target/i386/iinline-attr-2.c: New test.
>>
>> OK for mainline.
> 
> Hi Christian,
> I noticed case gcc.dg/ipa/iinline-attr.c failed on aarch64.  The
> original patch is x86 specific, while the case is added as general
> one.  Could you please have a look at this?
> 
> FAIL: gcc.dg/ipa/iinline-attr.c scan-ipa-dump inline
> "hooray[^\\n]*inline copy in test"
> 
> Thanks,
> bin
>>
>> Thanks,
>> Uros

Re: [PATCH, x86] Add TARGET_OVERRIDE_OPTIONS_AFTER_CHANGE hook

[Christian Bruel]

[-- Attachment #1: Type: text/plain, Size: 444 bytes --]


> Hi Christian,
> I noticed case gcc.dg/ipa/iinline-attr.c failed on aarch64.  The
> original patch is x86 specific, while the case is added as general
> one.  Could you please have a look at this?
> 
> FAIL: gcc.dg/ipa/iinline-attr.c scan-ipa-dump inline
> "hooray[^\\n]*inline copy in test"
> 

that is the same latent bug for aarch64:  alignment flags are not
propagated with attribute optimize ("O2").

testing attached patch

Christian



[-- Attachment #2: align.patch --]
[-- Type: text/x-patch, Size: 1248 bytes --]

Index: config/aarch64/aarch64.c
===================================================================
--- config/aarch64/aarch64.c	(revision 222627)
+++ config/aarch64/aarch64.c	(working copy)
@@ -6908,18 +6908,6 @@
 #endif
     }
 
-  /* If not opzimizing for size, set the default
-     alignment to what the target wants */
-  if (!optimize_size)
-    {
-      if (align_loops <= 0)
-	align_loops = aarch64_tune_params->loop_align;
-      if (align_jumps <= 0)
-	align_jumps = aarch64_tune_params->jump_align;
-      if (align_functions <= 0)
-	align_functions = aarch64_tune_params->function_align;
-    }
-
   if (AARCH64_TUNE_FMA_STEERING)
     aarch64_register_fma_steering ();
 
@@ -6935,6 +6923,18 @@
     flag_omit_leaf_frame_pointer = false;
   else if (flag_omit_leaf_frame_pointer)
     flag_omit_frame_pointer = true;
+
+  /* If not opzimizing for size, set the default
+     alignment to what the target wants */
+  if (!optimize_size)
+    {
+      if (align_loops <= 0)
+	align_loops = aarch64_tune_params->loop_align;
+      if (align_jumps <= 0)
+	align_jumps = aarch64_tune_params->jump_align;
+      if (align_functions <= 0)
+	align_functions = aarch64_tune_params->function_align;
+    }
 }
 
 static struct machine_function *

Re: [PATCH, x86] Add TARGET_OVERRIDE_OPTIONS_AFTER_CHANGE hook

[Yvan Roux]

Hi Christian,

On 4 May 2015 at 11:29, Christian Bruel <christian.bruel@st.com> wrote:
>
>> Hi Christian,
>> I noticed case gcc.dg/ipa/iinline-attr.c failed on aarch64.  The
>> original patch is x86 specific, while the case is added as general
>> one.  Could you please have a look at this?
>>
>> FAIL: gcc.dg/ipa/iinline-attr.c scan-ipa-dump inline
>> "hooray[^\\n]*inline copy in test"
>>
>
> that is the same latent bug for aarch64:  alignment flags are not
> propagated with attribute optimize ("O2").
>
> testing attached patch

The patch looks good to me, maybe you can just fix the original typo
on "optimizing" in the comments while moving the code.  I've
bootstrapped and regtested it on aarch64-linux-gnu with the same
target testcase you added on i386 (as we discussed offline) and
everything is ok (gcc.dg/ipa/iinline-attr.c now PASS).  Notice that I
can't approve your patch.

Cheers,
Yvan

Re: [PATCH, x86] Add TARGET_OVERRIDE_OPTIONS_AFTER_CHANGE hook

[Christian Bruel]

thanks for the validation and the confirmation that iinline-attr.c is
now fixed on aarch64. I can now send the patch for submission request
(this one was just illustrative).

thanks

Christian

On 05/05/2015 11:10 AM, Yvan Roux wrote:
> Hi Christian,
> 
> On 4 May 2015 at 11:29, Christian Bruel <christian.bruel@st.com> wrote:
>>
>>> Hi Christian,
>>> I noticed case gcc.dg/ipa/iinline-attr.c failed on aarch64.  The
>>> original patch is x86 specific, while the case is added as general
>>> one.  Could you please have a look at this?
>>>
>>> FAIL: gcc.dg/ipa/iinline-attr.c scan-ipa-dump inline
>>> "hooray[^\\n]*inline copy in test"
>>>
>>
>> that is the same latent bug for aarch64:  alignment flags are not
>> propagated with attribute optimize ("O2").
>>
>> testing attached patch
> 
> The patch looks good to me, maybe you can just fix the original typo
> on "optimizing" in the comments while moving the code.  I've
> bootstrapped and regtested it on aarch64-linux-gnu with the same
> target testcase you added on i386 (as we discussed offline) and
> everything is ok (gcc.dg/ipa/iinline-attr.c now PASS).  Notice that I
> can't approve your patch.
> 
> Cheers,
> Yvan
>