[PATCH][ARM] Fix PR target/69245 Rewrite arm_set_current_function

[PATCH][ARM] Fix PR target/69245 Rewrite arm_set_current_function

[Christian Bruel]

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

The current arm_set_current_function was both awkward and buggy. For 
instance using partially set TARGET_OPTION set from pragma_parse, while 
restore_target_globalsnor arm_option_params_internal was not reset. 
Another issue is that in some paths, target_reinit was not called due to 
old cached target_option_current_node value. for instance with

foo{}
#pragma GCC target ...

foo was called with global_options set from old GCC target (which was 
wrong) and correct rtl values.

This is a reimplementation of the function. Hoping to be easier to read 
(and maintain). Solves the current issues seen so far.

regtested for arm-linux-gnueabi -mfpu=vfp, -mfpu=neon,-mfpu=neon-fp-armv8





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

2016-01-20  Christian Bruel  <christian.bruel@st.com>

	PR target/69245
	* config/arm/arm-c.c (arm_pragma_target_parse): Add comments.
	Move arm_reset_previous_fndecl and set_target_option_current_node in the conditional part.
	* config/arm/arm.c (arm_set_current_function):
	Refactor to better support #pragma target and attribute mix.

2016-01-20  Christian Bruel  <christian.bruel@st.com>

	PR target/69245
	* gcc.target/arm/pr69245.c: New test.

Index: gcc/config/arm/arm-c.c
===================================================================
--- gcc/config/arm/arm-c.c	(revision 232669)
+++ gcc/config/arm/arm-c.c	(working copy)
@@ -221,9 +221,6 @@ arm_pragma_target_parse (tree args, tree pop_targe
 	}
     }
 
-  target_option_current_node = cur_tree;
-  arm_reset_previous_fndecl ();
-
   /* Figure out the previous mode.  */
   prev_opt  = TREE_TARGET_OPTION (prev_tree);
   cur_opt   = TREE_TARGET_OPTION (cur_tree);
@@ -261,8 +255,13 @@ arm_pragma_target_parse (tree args, tree pop_targe
 
       arm_cpu_builtins (parse_in);
 
       cpp_opts->warn_unused_macros = saved_warn_unused_macros;
+
+      /* Make sure that target_reinit is called for next function, since
+	 TREE_TARGET_OPTION might change with the #pragma even if there are
+	 no target attribute attached to the function.  */
+      target_option_current_node = cur_tree;
+      arm_reset_previous_fndecl ();
     }
 
   return true;
Index: gcc/config/arm/arm.c
===================================================================
--- gcc/config/arm/arm.c	(revision 232669)
+++ gcc/config/arm/arm.c	(working copy)
@@ -29768,35 +29768,28 @@ arm_set_current_function (tree fndecl)
 
   tree new_tree = DECL_FUNCTION_SPECIFIC_TARGET (fndecl);
 
-  arm_previous_fndecl = fndecl;
+  /* if no attribute, use the mode set by the current pragma target.  */
+  if (! new_tree)
+    new_tree = target_option_current_node;
+
   if (old_tree == new_tree)
     return;
 
-  if (new_tree && new_tree != target_option_default_node)
-    {
-      cl_target_option_restore (&global_options,
-				TREE_TARGET_OPTION (new_tree));
+  arm_previous_fndecl = fndecl;
 
-      if (TREE_TARGET_GLOBALS (new_tree))
-	restore_target_globals (TREE_TARGET_GLOBALS (new_tree));
-      else
-	TREE_TARGET_GLOBALS (new_tree)
-	  = save_target_globals_default_opts ();
-    }
+  /* First set the target options.  */
+  cl_target_option_restore (&global_options, TREE_TARGET_OPTION (new_tree));
 
-  else if (old_tree && old_tree != target_option_default_node)
+  /* If we have a previous state, use it.  */
+  if (TREE_TARGET_GLOBALS (new_tree))
+    restore_target_globals (TREE_TARGET_GLOBALS (new_tree));
+  else if (new_tree == target_option_default_node)
+    restore_target_globals (&default_target_globals);
+  else
     {
-      new_tree = target_option_current_node;
-
-      cl_target_option_restore (&global_options,
-				TREE_TARGET_OPTION (new_tree));
-      if (TREE_TARGET_GLOBALS (new_tree))
-	restore_target_globals (TREE_TARGET_GLOBALS (new_tree));
-      else if (new_tree == target_option_default_node)
-	restore_target_globals (&default_target_globals);
-      else
-	TREE_TARGET_GLOBALS (new_tree)
-	  = save_target_globals_default_opts ();
+      /* now target_reinit can save the state for later. */
+      TREE_TARGET_GLOBALS (new_tree)
+	= save_target_globals_default_opts ();
     }
 
   arm_option_params_internal ();
Index: gcc/testsuite/gcc.target/arm/pr69245.c
===================================================================
--- gcc/testsuite/gcc.target/arm/pr69245.c	(revision 0)
+++ gcc/testsuite/gcc.target/arm/pr69245.c	(working copy)
@@ -0,0 +1,24 @@
+/* PR target/69245 */
+/* Test that pop_options restores the vfp fpu mode.  */
+/* { dg-do compile } */
+/* { dg-require-effective-target arm_fp_ok } */
+/* { dg-add-options arm_fp } */
+
+#pragma GCC target "fpu=vfp"
+
+#pragma GCC push_options
+#pragma GCC target "fpu=neon"
+int a, c, d;
+float b;
+static int fn1 ()
+{
+  return 0;
+}
+#pragma GCC pop_options
+
+void fn2 ()
+{
+  d = b * c + a;
+}
+
+/* { dg-final { scan-assembler-times "\.fpu vfp" 1 } } */

Re: [PATCH][ARM] Fix PR target/69245 Rewrite arm_set_current_function

[Kyrill Tkachov]

Hi Christian,

On 21/01/16 10:36, Christian Bruel wrote:
> The current arm_set_current_function was both awkward and buggy. For instance using partially set TARGET_OPTION set from pragma_parse, while restore_target_globalsnor arm_option_params_internal was not reset. Another issue is that in some 
> paths, target_reinit was not called due to old cached target_option_current_node value. for instance with
>
> foo{}
> #pragma GCC target ...
>
> foo was called with global_options set from old GCC target (which was wrong) and correct rtl values.
>
> This is a reimplementation of the function. Hoping to be easier to read (and maintain). Solves the current issues seen so far.
>
> regtested for arm-linux-gnueabi -mfpu=vfp, -mfpu=neon,-mfpu=neon-fp-armv8
>

Thanks for the patch, I'll try it out.
In the meantime there's a couple of style and typo nits...

+      /* Make sure that target_reinit is called for next function, since
+	 TREE_TARGET_OPTION might change with the #pragma even if there are
+	 no target attribute attached to the function.  */

s/attribute/attributes

-  arm_previous_fndecl = fndecl;
+  /* if no attribute, use the mode set by the current pragma target.  */
+  if (! new_tree)
+    new_tree = target_option_current_node;
+

s/if/If/

+      /* now target_reinit can save the state for later. */
+      TREE_TARGET_GLOBALS (new_tree)
+	= save_target_globals_default_opts ();

s/now/Now/

Re: [PATCH][ARM] Fix PR target/69245 Rewrite arm_set_current_function

[Christian Bruel]

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

Hi Kyrill,

On 01/21/2016 01:22 PM, Kyrill Tkachov wrote:
> Hi Christian,
>
> On 21/01/16 10:36, Christian Bruel wrote:
>> The current arm_set_current_function was both awkward and buggy. For instance using partially set TARGET_OPTION set from pragma_parse, while restore_target_globalsnor arm_option_params_internal was not reset. Another issue is that in some
>> paths, target_reinit was not called due to old cached target_option_current_node value. for instance with
>>
>> foo{}
>> #pragma GCC target ...
>>
>> foo was called with global_options set from old GCC target (which was wrong) and correct rtl values.
>>
>> This is a reimplementation of the function. Hoping to be easier to read (and maintain). Solves the current issues seen so far.
>>
>> regtested for arm-linux-gnueabi -mfpu=vfp, -mfpu=neon,-mfpu=neon-fp-armv8
>>
> Thanks for the patch, I'll try it out.
> In the meantime there's a couple of style and typo nits...
>
> +      /* Make sure that target_reinit is called for next function, since
> +	 TREE_TARGET_OPTION might change with the #pragma even if there are
> +	 no target attribute attached to the function.  */
>
> s/attribute/attributes
>
> -  arm_previous_fndecl = fndecl;
> +  /* if no attribute, use the mode set by the current pragma target.  */
> +  if (! new_tree)
> +    new_tree = target_option_current_node;
> +
>
> s/if/If/
>
> +      /* now target_reinit can save the state for later. */
> +      TREE_TARGET_GLOBALS (new_tree)
> +	= save_target_globals_default_opts ();
>
> s/now/Now/
>

While playing on my side. I realized that we could simplify the patch 
further by removing the need to set and use target_option_current_node, 
since this is redundant with what handle_pragma_push/pop_options does.
Also since that the functions inside a pragma GCC target region will 
have DECL_FUNCTION_SPECIFIC_TARGET set already, we don't seem to need a 
special case for those.

With this V2, arm_set_current_function is becoming more minimalist and 
still fixes the current issues. Could you test this version instead ?

Also, I see that in the i386 implementation (CCed Jacub) that 
target_option_current_node also set old_tree. I'm wondering if this 
isn't covering the case where ix86_previous_fndecl is NULL (reset by 
pragma), with some redundancy with the generic code in c-pragma.c ?











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

2016-01-20  Christian Bruel  <christian.bruel@st.com>

	PR target/69245
	* config/arm/arm-c.c (arm_pragma_target_parse): Add comments.
	Call arm_reset_previous_fndecl conditionally.
	* config/arm/arm.c (arm_set_current_function):
	Refactor to better support #pragma target and attribute mix.
	(arm_option_override): Remove target_option_current_node setting.

2016-01-20  Christian Bruel  <christian.bruel@st.com>

	PR target/69245
	* gcc.target/arm/pr69245.c: New test.

Index: gcc/config/arm/arm-c.c
===================================================================
--- gcc/config/arm/arm-c.c	(revision 232726)
+++ gcc/config/arm/arm-c.c	(working copy)
@@ -221,9 +221,6 @@ arm_pragma_target_parse (tree args, tree pop_targe
 	}
     }
 
-  target_option_current_node = cur_tree;
-  arm_reset_previous_fndecl ();
-
   /* Figure out the previous mode.  */
   prev_opt  = TREE_TARGET_OPTION (prev_tree);
   cur_opt   = TREE_TARGET_OPTION (cur_tree);
@@ -259,6 +256,11 @@ arm_pragma_target_parse (tree args, tree pop_targe
       arm_cpu_builtins (parse_in);
 
       cpp_opts->warn_unused_macros = saved_warn_unused_macros;
+
+      /* Make sure that target_reinit is called for next function, since
+	 TREE_TARGET_OPTION might change with the #pragma even if there is
+	 no target attribute attached to the function.  */
+      arm_reset_previous_fndecl ();
     }
 
   return true;
Index: gcc/config/arm/arm.c
===================================================================
--- gcc/config/arm/arm.c	(revision 232726)
+++ gcc/config/arm/arm.c	(working copy)
@@ -3446,8 +3446,7 @@ arm_option_override (void)
 
   /* Save the initial options in case the user does function specific
      options.  */
-  target_option_default_node = target_option_current_node
-    = build_target_option_node (&global_options);
+  target_option_default_node = build_target_option_node (&global_options);
 
   /* Init initial mode for testing.  */
   thumb_flipper = TARGET_THUMB;
@@ -29768,35 +29767,29 @@ arm_set_current_function (tree fndecl)
 
   tree new_tree = DECL_FUNCTION_SPECIFIC_TARGET (fndecl);
 
-  arm_previous_fndecl = fndecl;
+  /* if no attribute, but there was one. Use the default mode.  */
+  if (! new_tree && old_tree)
+    new_tree = target_option_default_node;
+
   if (old_tree == new_tree)
     return;
 
-  if (new_tree && new_tree != target_option_default_node)
-    {
-      cl_target_option_restore (&global_options,
-				TREE_TARGET_OPTION (new_tree));
+  arm_previous_fndecl = fndecl;
 
-      if (TREE_TARGET_GLOBALS (new_tree))
-	restore_target_globals (TREE_TARGET_GLOBALS (new_tree));
-      else
-	TREE_TARGET_GLOBALS (new_tree)
-	  = save_target_globals_default_opts ();
-    }
+  /* First set the target options.  */
+  cl_target_option_restore (&global_options, TREE_TARGET_OPTION (new_tree));
 
-  else if (old_tree && old_tree != target_option_default_node)
+  /* If we have a previous state, use it.  */
+  if (TREE_TARGET_GLOBALS (new_tree))
+    restore_target_globals (TREE_TARGET_GLOBALS (new_tree));
+  else if (new_tree == target_option_default_node)
+    restore_target_globals (&default_target_globals);
+  else
     {
-      new_tree = target_option_current_node;
-
-      cl_target_option_restore (&global_options,
-				TREE_TARGET_OPTION (new_tree));
-      if (TREE_TARGET_GLOBALS (new_tree))
-	restore_target_globals (TREE_TARGET_GLOBALS (new_tree));
-      else if (new_tree == target_option_default_node)
-	restore_target_globals (&default_target_globals);
-      else
-	TREE_TARGET_GLOBALS (new_tree)
-	  = save_target_globals_default_opts ();
+      /* Call target_reinit and save the state for TARGET_GLOBALS. */
+      TREE_TARGET_GLOBALS (new_tree) = save_target_globals_default_opts ();
     }
 
   arm_option_params_internal ();
Index: gcc/testsuite/gcc.target/arm/pr69245.c
===================================================================
--- gcc/testsuite/gcc.target/arm/pr69245.c	(revision 0)
+++ gcc/testsuite/gcc.target/arm/pr69245.c	(working copy)
@@ -0,0 +1,24 @@
+/* PR target/69245 */
+/* Test that pop_options restores the vfp fpu mode.  */
+/* { dg-do compile } */
+/* { dg-require-effective-target arm_fp_ok } */
+/* { dg-add-options arm_fp } */
+
+#pragma GCC target "fpu=vfp"
+
+#pragma GCC push_options
+#pragma GCC target "fpu=neon"
+int a, c, d;
+float b;
+static int fn1 ()
+{
+  return 0;
+}
+#pragma GCC pop_options
+
+void fn2 ()
+{
+  d = b * c + a;
+}
+
+/* { dg-final { scan-assembler-times "\.fpu vfp" 1 } } */

Re: [PATCH][ARM] Fix PR target/69245 Rewrite arm_set_current_function

[Kyrill Tkachov]

Hi Christian,

On 22/01/16 14:07, Christian Bruel wrote:
> Hi Kyrill,
>
> On 01/21/2016 01:22 PM, Kyrill Tkachov wrote:
>> Hi Christian,
>>
>> On 21/01/16 10:36, Christian Bruel wrote:
>>> The current arm_set_current_function was both awkward and buggy. For instance using partially set TARGET_OPTION set from pragma_parse, while restore_target_globalsnor arm_option_params_internal was not reset. Another issue is that in some
>>> paths, target_reinit was not called due to old cached target_option_current_node value. for instance with
>>>
>>> foo{}
>>> #pragma GCC target ...
>>>
>>> foo was called with global_options set from old GCC target (which was wrong) and correct rtl values.
>>>
>>> This is a reimplementation of the function. Hoping to be easier to read (and maintain). Solves the current issues seen so far.
>>>
>>> regtested for arm-linux-gnueabi -mfpu=vfp, -mfpu=neon,-mfpu=neon-fp-armv8
>>>
>> Thanks for the patch, I'll try it out.
>> In the meantime there's a couple of style and typo nits...
>>
>> +      /* Make sure that target_reinit is called for next function, since
>> +     TREE_TARGET_OPTION might change with the #pragma even if there are
>> +     no target attribute attached to the function.  */
>>
>> s/attribute/attributes
>>
>> -  arm_previous_fndecl = fndecl;
>> +  /* if no attribute, use the mode set by the current pragma target.  */
>> +  if (! new_tree)
>> +    new_tree = target_option_current_node;
>> +
>>
>> s/if/If/
>>
>> +      /* now target_reinit can save the state for later. */
>> +      TREE_TARGET_GLOBALS (new_tree)
>> +    = save_target_globals_default_opts ();
>>
>> s/now/Now/
>>
>
> While playing on my side. I realized that we could simplify the patch further by removing the need to set and use target_option_current_node, since this is redundant with what handle_pragma_push/pop_options does.
> Also since that the functions inside a pragma GCC target region will have DECL_FUNCTION_SPECIFIC_TARGET set already, we don't seem to need a special case for those.
>
> With this V2, arm_set_current_function is becoming more minimalist and still fixes the current issues. Could you test this version instead ?
>

Thanks, I'll check this out instead.
I've played a bit with your previous version and the effect on the testcases looked ok, but I have a couple of
comments on the testcase in the meantime

Index: gcc/testsuite/gcc.target/arm/pr69245.c
===================================================================
--- gcc/testsuite/gcc.target/arm/pr69245.c	(revision 0)
+++ gcc/testsuite/gcc.target/arm/pr69245.c	(working copy)
@@ -0,0 +1,24 @@
+/* PR target/69245 */
+/* Test that pop_options restores the vfp fpu mode.  */
+/* { dg-do compile } */
+/* { dg-require-effective-target arm_fp_ok } */
+/* { dg-add-options arm_fp } */
+
+#pragma GCC target "fpu=vfp"
+
+#pragma GCC push_options
+#pragma GCC target "fpu=neon"
+int a, c, d;
+float b;
+static int fn1 ()
+{
+  return 0;
+}
+#pragma GCC pop_options
+
+void fn2 ()
+{
+  d = b * c + a;
+}
+
+/* { dg-final { scan-assembler-times "\.fpu vfp" 1 } } */


PR 69245 is an ICE whereas your testcase doesn't ICE without the patch, it just fails the
scan-assembler check. I'd like to have the testcase trigger the ICE without your patch.
For that we need -O2 in dg-options.
Also, the "fpu=vfp" pragma you put in the beginning doesn't allow the ICE to trigger, presumably
because it triggers a different path through the pragma option popping code.
So removing that pragma and instead changing the dg-add-options from arm_fp to arm_vfp3 (which is
floating-point without the vfma instruction causes the ICE) does the trick for me.
Also the "fpu=neon" pragma should also be changed to be "fpu=neon-vfpv4" because that setting allows
the vfma instruction which is being wrongly considered in fn2().
I suppose you'll then want to change the scan-assembler directive to look for \.fpu vfp3.

Thanks,
Kyrill

Re: [PATCH][ARM] Fix PR target/69245 Rewrite arm_set_current_function

[Christian Bruel]

Hi Kyrill,

On 01/22/2016 03:17 PM, Kyrill Tkachov wrote:
> Hi Christian,
>
> On 22/01/16 14:07, Christian Bruel wrote:
>> Hi Kyrill,
>>
>> On 01/21/2016 01:22 PM, Kyrill Tkachov wrote:
>>> Hi Christian,
>>>
>>> On 21/01/16 10:36, Christian Bruel wrote:
>>>> The current arm_set_current_function was both awkward and buggy. For instance using partially set TARGET_OPTION set from pragma_parse, while restore_target_globalsnor arm_option_params_internal was not reset. Another issue is that in some
>>>> paths, target_reinit was not called due to old cached target_option_current_node value. for instance with
>>>>
>>>> foo{}
>>>> #pragma GCC target ...
>>>>
>>>> foo was called with global_options set from old GCC target (which was wrong) and correct rtl values.
>>>>
>>>> This is a reimplementation of the function. Hoping to be easier to read (and maintain). Solves the current issues seen so far.
>>>>
>>>> regtested for arm-linux-gnueabi -mfpu=vfp, -mfpu=neon,-mfpu=neon-fp-armv8
>>>>
>>> Thanks for the patch, I'll try it out.
>>> In the meantime there's a couple of style and typo nits...
>>>
>>> +      /* Make sure that target_reinit is called for next function, since
>>> +     TREE_TARGET_OPTION might change with the #pragma even if there are
>>> +     no target attribute attached to the function.  */
>>>
>>> s/attribute/attributes
>>>
>>> -  arm_previous_fndecl = fndecl;
>>> +  /* if no attribute, use the mode set by the current pragma target.  */
>>> +  if (! new_tree)
>>> +    new_tree = target_option_current_node;
>>> +
>>>
>>> s/if/If/
>>>
>>> +      /* now target_reinit can save the state for later. */
>>> +      TREE_TARGET_GLOBALS (new_tree)
>>> +    = save_target_globals_default_opts ();
>>>
>>> s/now/Now/
>>>
>> While playing on my side. I realized that we could simplify the patch further by removing the need to set and use target_option_current_node, since this is redundant with what handle_pragma_push/pop_options does.
>> Also since that the functions inside a pragma GCC target region will have DECL_FUNCTION_SPECIFIC_TARGET set already, we don't seem to need a special case for those.
>>
>> With this V2, arm_set_current_function is becoming more minimalist and still fixes the current issues. Could you test this version instead ?
>>
> Thanks, I'll check this out instead.
> I've played a bit with your previous version and the effect on the testcases looked ok, but I have a couple of
> comments on the testcase in the meantime
>
> Index: gcc/testsuite/gcc.target/arm/pr69245.c
> ===================================================================
> --- gcc/testsuite/gcc.target/arm/pr69245.c	(revision 0)
> +++ gcc/testsuite/gcc.target/arm/pr69245.c	(working copy)
> @@ -0,0 +1,24 @@
> +/* PR target/69245 */
> +/* Test that pop_options restores the vfp fpu mode.  */
> +/* { dg-do compile } */
> +/* { dg-require-effective-target arm_fp_ok } */
> +/* { dg-add-options arm_fp } */
> +
> +#pragma GCC target "fpu=vfp"
> +
> +#pragma GCC push_options
> +#pragma GCC target "fpu=neon"
> +int a, c, d;
> +float b;
> +static int fn1 ()
> +{
> +  return 0;
> +}
> +#pragma GCC pop_options
> +
> +void fn2 ()
> +{
> +  d = b * c + a;
> +}
> +
> +/* { dg-final { scan-assembler-times "\.fpu vfp" 1 } } */
>
>
> PR 69245 is an ICE whereas your testcase doesn't ICE without the patch, it just fails the
> scan-assembler check. I'd like to have the testcase trigger the ICE without your patch.
> For that we need -O2 in dg-options.
> Also, the "fpu=vfp" pragma you put in the beginning doesn't allow the ICE to trigger, presumably
> because it triggers a different path through the pragma option popping code.
> So removing that pragma and instead changing the dg-add-options from arm_fp to arm_vfp3 (which is
> floating-point without the vfma instruction causes the ICE) does the trick for me.
> Also the "fpu=neon" pragma should also be changed to be "fpu=neon-vfpv4" because that setting allows
> the vfma instruction which is being wrongly considered in fn2().
> I suppose you'll then want to change the scan-assembler directive to look for \.fpu vfp3.

ah yes ! OK for -O2, I thought I had it, must have been deleted 
somewhere :-(

I added the #pragma GCC target "fpu=vfp" to have some kind of 
deterministic checks to guard against the options permutations that 
Christophe stresses during his validations. so for instance the ".fpu 
scan-assembler would change depending on the default options...

so the following test should ICE with the all configurations 
(!-mfloat-abi=soft) in -O2

#pragma GCC target "fpu=vfp"

#pragma GCC push_options
#pragma GCC target "fpu=neon-vfpv4"
int a, c, d;
float b;
static int fn1 ()
{
   return 0;
}

#pragma GCC pop_options
void fn2 ()
{
   d = b * c + a;
}


I'm also continuing to playing on my side with different scenarios for 
mixtures of push/pop/reset attributes.

thanks

Christian
>
> Thanks,
> Kyrill
>

Re: [PATCH][ARM] Fix PR target/69245 Rewrite arm_set_current_function

[Kyrill Tkachov]

On 22/01/16 14:51, Christian Bruel wrote:
> Hi Kyrill,
>
> On 01/22/2016 03:17 PM, Kyrill Tkachov wrote:
>> Hi Christian,
>>
>> On 22/01/16 14:07, Christian Bruel wrote:
>>> Hi Kyrill,
>>>
>>> On 01/21/2016 01:22 PM, Kyrill Tkachov wrote:
>>>> Hi Christian,
>>>>
>>>> On 21/01/16 10:36, Christian Bruel wrote:
>>>>> The current arm_set_current_function was both awkward and buggy. For instance using partially set TARGET_OPTION set from pragma_parse, while restore_target_globalsnor arm_option_params_internal was not reset. Another issue is that in 
>>>>> some
>>>>> paths, target_reinit was not called due to old cached target_option_current_node value. for instance with
>>>>>
>>>>> foo{}
>>>>> #pragma GCC target ...
>>>>>
>>>>> foo was called with global_options set from old GCC target (which was wrong) and correct rtl values.
>>>>>
>>>>> This is a reimplementation of the function. Hoping to be easier to read (and maintain). Solves the current issues seen so far.
>>>>>
>>>>> regtested for arm-linux-gnueabi -mfpu=vfp, -mfpu=neon,-mfpu=neon-fp-armv8
>>>>>
>>>> Thanks for the patch, I'll try it out.
>>>> In the meantime there's a couple of style and typo nits...
>>>>
>>>> +      /* Make sure that target_reinit is called for next function, since
>>>> +     TREE_TARGET_OPTION might change with the #pragma even if there are
>>>> +     no target attribute attached to the function.  */
>>>>
>>>> s/attribute/attributes
>>>>
>>>> -  arm_previous_fndecl = fndecl;
>>>> +  /* if no attribute, use the mode set by the current pragma target.  */
>>>> +  if (! new_tree)
>>>> +    new_tree = target_option_current_node;
>>>> +
>>>>
>>>> s/if/If/
>>>>
>>>> +      /* now target_reinit can save the state for later. */
>>>> +      TREE_TARGET_GLOBALS (new_tree)
>>>> +    = save_target_globals_default_opts ();
>>>>
>>>> s/now/Now/
>>>>
>>> While playing on my side. I realized that we could simplify the patch further by removing the need to set and use target_option_current_node, since this is redundant with what handle_pragma_push/pop_options does.
>>> Also since that the functions inside a pragma GCC target region will have DECL_FUNCTION_SPECIFIC_TARGET set already, we don't seem to need a special case for those.
>>>
>>> With this V2, arm_set_current_function is becoming more minimalist and still fixes the current issues. Could you test this version instead ?
>>>
>> Thanks, I'll check this out instead.
>> I've played a bit with your previous version and the effect on the testcases looked ok, but I have a couple of
>> comments on the testcase in the meantime
>>
>> Index: gcc/testsuite/gcc.target/arm/pr69245.c
>> ===================================================================
>> --- gcc/testsuite/gcc.target/arm/pr69245.c    (revision 0)
>> +++ gcc/testsuite/gcc.target/arm/pr69245.c    (working copy)
>> @@ -0,0 +1,24 @@
>> +/* PR target/69245 */
>> +/* Test that pop_options restores the vfp fpu mode.  */
>> +/* { dg-do compile } */
>> +/* { dg-require-effective-target arm_fp_ok } */
>> +/* { dg-add-options arm_fp } */
>> +
>> +#pragma GCC target "fpu=vfp"
>> +
>> +#pragma GCC push_options
>> +#pragma GCC target "fpu=neon"
>> +int a, c, d;
>> +float b;
>> +static int fn1 ()
>> +{
>> +  return 0;
>> +}
>> +#pragma GCC pop_options
>> +
>> +void fn2 ()
>> +{
>> +  d = b * c + a;
>> +}
>> +
>> +/* { dg-final { scan-assembler-times "\.fpu vfp" 1 } } */
>>
>>
>> PR 69245 is an ICE whereas your testcase doesn't ICE without the patch, it just fails the
>> scan-assembler check. I'd like to have the testcase trigger the ICE without your patch.
>> For that we need -O2 in dg-options.
>> Also, the "fpu=vfp" pragma you put in the beginning doesn't allow the ICE to trigger, presumably
>> because it triggers a different path through the pragma option popping code.
>> So removing that pragma and instead changing the dg-add-options from arm_fp to arm_vfp3 (which is
>> floating-point without the vfma instruction causes the ICE) does the trick for me.
>> Also the "fpu=neon" pragma should also be changed to be "fpu=neon-vfpv4" because that setting allows
>> the vfma instruction which is being wrongly considered in fn2().
>> I suppose you'll then want to change the scan-assembler directive to look for \.fpu vfp3.
>
> ah yes ! OK for -O2, I thought I had it, must have been deleted somewhere :-(
>
> I added the #pragma GCC target "fpu=vfp" to have some kind of deterministic checks to guard against the options permutations that Christophe stresses during his validations. so for instance the ".fpu scan-assembler would change depending 
> on the default options...
>
> so the following test should ICE with the all configurations (!-mfloat-abi=soft) in -O2
>
> #pragma GCC target "fpu=vfp"
>
> #pragma GCC push_options
> #pragma GCC target "fpu=neon-vfpv4"
> int a, c, d;
> float b;
> static int fn1 ()
> {
>   return 0;
> }
>
> #pragma GCC pop_options
> void fn2 ()
> {
>   d = b * c + a;
> }
>
>

Ah ok, I needed to update my tree to include your other midend fixes in this area.
I played around with the patch and gave it a bootstrap as well.
I wanted to make a sanity check on compile-time performance for files using arm_neon.h
and I didn't spot any measurable regressions.

So this is ok for trunk with the testcase changed as discussed above and using -O2
optimisation level and with a couple comment fixes below.

-  arm_previous_fndecl = fndecl;
+  /* if no attribute, but there was one. Use the default mode.  */
+  if (! new_tree && old_tree)
+    new_tree = target_option_default_node;
+


Start with capital 'I'. Maybe phrase it as:
"If current function has no attributes but previous one did, use the default node."  ?

+      /* Call target_reinit and save the state for TARGET_GLOBALS. */
+      TREE_TARGET_GLOBALS (new_tree) = save_target_globals_default_opts ();

Two spaces between full stop and comment closure.

Thanks,
Kyrill

Re: [PATCH][ARM] Fix PR target/69245 Rewrite arm_set_current_function

[Christian Bruel]

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



On 01/25/2016 05:37 PM, Kyrill Tkachov wrote:


So this is ok for trunk with the testcase changed as discussed above and using -O2
optimisation level and with a couple comment fixes below.

Hi Kyrill,

I realized afterwards that my implementation had a flaw with the 
handling of #pragma GCC reset. What happened is that when both old and 
new TREE_TARGET_OPTION are NULL, we didn't save_restore the globals 
flags, to save compute time. The problem is that with #pragma GCC reset, 
we also fall into this situation, and exit without calling 
target_reeinit :-(

Handling this situation doesn't complicate the code much, because I 
factorized the calls to target_reeinit + restore_target_globals into a 
new function (save_restore_target_globals). This function is called from 
the pragma context when resetting the state arm_reset_previous_fndecl to 
the default, and from arm_set_current_function when setting to a new 
target. This is only done when there is a change of the target flags, so 
no extra computing cost.

Same testing as with previous patch:
     arm-qemu/
     arm-qemu//-mfpu=neon-fp-armv8
     arm-qemu//-mfpu=neon

Still OK ?







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

2016-01-20  Christian Bruel  <christian.bruel@st.com>

	PR target/69245
	* config/arm/arm-c.c (arm_pragma_target_parse): Add comments.
	Move arm_reset_previous_fndecl and set_target_option_current_node in
	the conditional part. Call save_restore_target_globals.
	* config/arm/arm.c (arm_set_current_function):
	Refactor to better support #pragma target and attribute mix.
	Call save_restore_target_globals.
	* config/arm/arm-protos.h (arm_reset_previous_fndecl): Add parameter.
	(save_restore_target_globals): New function.

2016-01-20  Christian Bruel  <christian.bruel@st.com>

	PR target/69245
	* gcc.target/arm/pr69245.c: New test.

Index: gcc/config/arm/arm-c.c
===================================================================
--- gcc/config/arm/arm-c.c	(revision 232831)
+++ gcc/config/arm/arm-c.c	(working copy)
@@ -221,9 +221,6 @@ arm_pragma_target_parse (tree args, tree pop_targe
 	}
     }
 
-  target_option_current_node = cur_tree;
-  arm_reset_previous_fndecl ();
-
   /* Figure out the previous mode.  */
   prev_opt  = TREE_TARGET_OPTION (prev_tree);
   cur_opt   = TREE_TARGET_OPTION (cur_tree);
@@ -259,6 +256,13 @@ arm_pragma_target_parse (tree args, tree pop_targe
       arm_cpu_builtins (parse_in);
 
       cpp_opts->warn_unused_macros = saved_warn_unused_macros;
+
+      /* Make sure that target_reinit is called for next function, since
+	 TREE_TARGET_OPTION might change with the #pragma even if there is
+	 no target attribute attached to the function.  */
+      if (cur_tree != target_option_default_node)
+	cur_tree = NULL;
+      arm_reset_previous_fndecl (cur_tree);
     }
 
   return true;
Index: gcc/config/arm/arm-protos.h
===================================================================
--- gcc/config/arm/arm-protos.h	(revision 232831)
+++ gcc/config/arm/arm-protos.h	(working copy)
@@ -331,7 +331,7 @@ extern bool arm_autoinc_modes_ok_p (machine_mode,
 
 extern void arm_emit_eabi_attribute (const char *, int, int);
 
-extern void arm_reset_previous_fndecl (void);
+extern void arm_reset_previous_fndecl (tree);
 
 /* Defined in gcc/common/config/arm-common.c.  */
 extern const char *arm_rewrite_selected_cpu (const char *name);
Index: gcc/config/arm/arm.c
===================================================================
--- gcc/config/arm/arm.c	(revision 232831)
+++ gcc/config/arm/arm.c	(working copy)
@@ -3446,8 +3446,7 @@ arm_option_override (void)
 
   /* Save the initial options in case the user does function specific
      options.  */
-  target_option_default_node = target_option_current_node
-    = build_target_option_node (&global_options);
+  target_option_default_node = build_target_option_node (&global_options);
 
   /* Init initial mode for testing.  */
   thumb_flipper = TARGET_THUMB;
@@ -29746,10 +29745,31 @@ arm_is_constant_pool_ref (rtx x)
 /* Remember the last target of arm_set_current_function.  */
 static GTY(()) tree arm_previous_fndecl;
 
+/* Restore the TREE_TARGET_GLOBALS from previous state, or save it.  */
+static void
+save_restore_target_globals (tree new_tree)
+{
+  /* If we have a previous state, use it.  */
+  if (TREE_TARGET_GLOBALS (new_tree))
+    restore_target_globals (TREE_TARGET_GLOBALS (new_tree));
+  else if (new_tree == target_option_default_node)
+    restore_target_globals (&default_target_globals);
+  else
+    {
+      /* Call target_reinit and save the state for TARGET_GLOBALS.  */
+      TREE_TARGET_GLOBALS (new_tree) = save_target_globals_default_opts ();
+    }
+
+  arm_option_params_internal ();
+}
+
 /* Invalidate arm_previous_fndecl.  */
 void
-arm_reset_previous_fndecl (void)
+arm_reset_previous_fndecl (tree new_tree)
 {
+  if (new_tree)
+    save_restore_target_globals (new_tree);
+
   arm_previous_fndecl = NULL_TREE;
 }
 
@@ -29768,38 +29788,23 @@ arm_set_current_function (tree fndecl)
 
   tree new_tree = DECL_FUNCTION_SPECIFIC_TARGET (fndecl);
 
-  arm_previous_fndecl = fndecl;
+  /* If current function has no attributes but previous one did,
+     use the default node."  */
+  if (! new_tree && old_tree)
+    new_tree = target_option_default_node;
+
+  /* If nothing to do return. #pragma GCC reset or #pragma GCC pop to
+     the default have been handled by save_restore_target_globals from
+     arm_pragma_target_parse.  */
   if (old_tree == new_tree)
     return;
 
-  if (new_tree && new_tree != target_option_default_node)
-    {
-      cl_target_option_restore (&global_options,
-				TREE_TARGET_OPTION (new_tree));
+  arm_previous_fndecl = fndecl;
 
-      if (TREE_TARGET_GLOBALS (new_tree))
-	restore_target_globals (TREE_TARGET_GLOBALS (new_tree));
-      else
-	TREE_TARGET_GLOBALS (new_tree)
-	  = save_target_globals_default_opts ();
-    }
+  /* First set the target options.  */
+  cl_target_option_restore (&global_options, TREE_TARGET_OPTION (new_tree));
 
-  else if (old_tree && old_tree != target_option_default_node)
-    {
-      new_tree = target_option_current_node;
-
-      cl_target_option_restore (&global_options,
-				TREE_TARGET_OPTION (new_tree));
-      if (TREE_TARGET_GLOBALS (new_tree))
-	restore_target_globals (TREE_TARGET_GLOBALS (new_tree));
-      else if (new_tree == target_option_default_node)
-	restore_target_globals (&default_target_globals);
-      else
-	TREE_TARGET_GLOBALS (new_tree)
-	  = save_target_globals_default_opts ();
-    }
-
-  arm_option_params_internal ();
+  save_restore_target_globals (new_tree);
 }
 
 /* Implement TARGET_OPTION_PRINT.  */
Index: gcc/testsuite/gcc.target/arm/pr69245.c
===================================================================
--- gcc/testsuite/gcc.target/arm/pr69245.c	(revision 0)
+++ gcc/testsuite/gcc.target/arm/pr69245.c	(working copy)
@@ -0,0 +1,26 @@
+/* PR target/69245 */
+/* Test that pop_options restores the vfp fpu mode.  */
+/* { dg-do compile } */
+/* { dg-require-effective-target arm_neon_ok } */
+/* { dg-require-effective-target arm_fp_ok } */
+/* { dg-options "-O2" } */
+/* { dg-add-options arm_fp } */
+
+#pragma GCC target "fpu=vfp"
+
+#pragma GCC push_options
+#pragma GCC target "fpu=neon-vfpv4"
+int a, c, d;
+float b;
+static int fn1 ()
+{
+  return 0;
+}
+#pragma GCC pop_options
+
+void fn2 ()
+{
+  d = b * c + a;
+}
+
+/* { dg-final { scan-assembler-times "\.fpu vfp" 1 } } */

Re: [PATCH][ARM] Fix PR target/69245 Rewrite arm_set_current_function

[Kyrill Tkachov]

Hi Christian,

On 26/01/16 15:29, Christian Bruel wrote:
>
>
> On 01/25/2016 05:37 PM, Kyrill Tkachov wrote:
>
>
> So this is ok for trunk with the testcase changed as discussed above and using -O2
> optimisation level and with a couple comment fixes below.
>
> Hi Kyrill,
>
> I realized afterwards that my implementation had a flaw with the handling of #pragma GCC reset. What happened is that when both old and new TREE_TARGET_OPTION are NULL, we didn't save_restore the globals flags, to save compute time. The 
> problem is that with #pragma GCC reset, we also fall into this situation, and exit without calling target_reeinit :-(
>
> Handling this situation doesn't complicate the code much, because I factorized the calls to target_reeinit + restore_target_globals into a new function (save_restore_target_globals). This function is called from the pragma context when 
> resetting the state arm_reset_previous_fndecl to the default, and from arm_set_current_function when setting to a new target. This is only done when there is a change of the target flags, so no extra computing cost.
>
> Same testing as with previous patch:
>     arm-qemu/
>     arm-qemu//-mfpu=neon-fp-armv8
>     arm-qemu//-mfpu=neon
>
> Still OK ?
>

+/* Restore the TREE_TARGET_GLOBALS from previous state, or save it.  */
+static void
+save_restore_target_globals (tree new_tree)
+{
+  /* If we have a previous state, use it.  */
+  if (TREE_TARGET_GLOBALS (new_tree))
+    restore_target_globals (TREE_TARGET_GLOBALS (new_tree));
+  else if (new_tree == target_option_default_node)
+    restore_target_globals (&default_target_globals);
+  else
+    {
+      /* Call target_reinit and save the state for TARGET_GLOBALS.  */
+      TREE_TARGET_GLOBALS (new_tree) = save_target_globals_default_opts ();
+    }
+
+  arm_option_params_internal ();
+}

Space before the function comment and signature. Also, you need to document what is the NEW_TREE
parameter.

  /* Invalidate arm_previous_fndecl.  */
  void
-arm_reset_previous_fndecl (void)
+arm_reset_previous_fndecl (tree new_tree)
  {
+  if (new_tree)
+    save_restore_target_globals (new_tree);
+
    arm_previous_fndecl = NULL_TREE;
  }

I'm a bit wary of complicating this function. Suddenly it doesn't just reset the previous fndecl
but also restores globals and can save stuff into its argument. It's suddenly not clear what it's
purpose is.
I think it would be clearer if you just added save_restore_target_globals to arm_protos.h and called
it explicitly from arm_pragma_target_parse when appropriate.

+
+  /* If nothing to do return. #pragma GCC reset or #pragma GCC pop to
+     the default have been handled by save_restore_target_globals from
+     arm_pragma_target_parse.  */

Two spaces between fullstop and "#pragma GCC".

Thanks,
Kyrill

Re: [PATCH][ARM] Fix PR target/69245 Rewrite arm_set_current_function

[Christian Bruel]

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



On 01/26/2016 04:58 PM, Kyrill Tkachov wrote:
> Hi Christian,
>
> On 26/01/16 15:29, Christian Bruel wrote:
>>
>> On 01/25/2016 05:37 PM, Kyrill Tkachov wrote:
>>
>>
>> So this is ok for trunk with the testcase changed as discussed above and using -O2
>> optimisation level and with a couple comment fixes below.
>>
>> Hi Kyrill,
>>
>> I realized afterwards that my implementation had a flaw with the handling of #pragma GCC reset. What happened is that when both old and new TREE_TARGET_OPTION are NULL, we didn't save_restore the globals flags, to save compute time. The
>> problem is that with #pragma GCC reset, we also fall into this situation, and exit without calling target_reeinit :-(
>>
>> Handling this situation doesn't complicate the code much, because I factorized the calls to target_reeinit + restore_target_globals into a new function (save_restore_target_globals). This function is called from the pragma context when
>> resetting the state arm_reset_previous_fndecl to the default, and from arm_set_current_function when setting to a new target. This is only done when there is a change of the target flags, so no extra computing cost.
>>
>> Same testing as with previous patch:
>>      arm-qemu/
>>      arm-qemu//-mfpu=neon-fp-armv8
>>      arm-qemu//-mfpu=neon
>>
>> Still OK ?
>>
> +/* Restore the TREE_TARGET_GLOBALS from previous state, or save it.  */
> +static void
> +save_restore_target_globals (tree new_tree)
> +{
> +  /* If we have a previous state, use it.  */
> +  if (TREE_TARGET_GLOBALS (new_tree))
> +    restore_target_globals (TREE_TARGET_GLOBALS (new_tree));
> +  else if (new_tree == target_option_default_node)
> +    restore_target_globals (&default_target_globals);
> +  else
> +    {
> +      /* Call target_reinit and save the state for TARGET_GLOBALS.  */
> +      TREE_TARGET_GLOBALS (new_tree) = save_target_globals_default_opts ();
> +    }
> +
> +  arm_option_params_internal ();
> +}
>
> Space before the function comment and signature.
what do you mean ?  a new line between the comment and the function 
signature ? I usually mimic what's done in the other arm.c declarations, 
which sometimes have a new line, sometime not. It's not clear to me 
what's mandatory here, even in the other parts of the compiler.

>   Also, you need to document what is the NEW_TREE
> parameter.
>
>    /* Invalidate arm_previous_fndecl.  */
>    void
> -arm_reset_previous_fndecl (void)
> +arm_reset_previous_fndecl (tree new_tree)
>    {
> +  if (new_tree)
> +    save_restore_target_globals (new_tree);
> +
>      arm_previous_fndecl = NULL_TREE;
>    }
>
> I'm a bit wary of complicating this function. Suddenly it doesn't just reset the previous fndecl
> but also restores globals and can save stuff into its argument. It's suddenly not clear what it's
> purpose is.
> I think it would be clearer if you just added save_restore_target_globals to arm_protos.h and called
> it explicitly from arm_pragma_target_parse when appropriate.

sure, like this one attached (sanity checked) ?


>
> +
> +  /* If nothing to do return. #pragma GCC reset or #pragma GCC pop to
> +     the default have been handled by save_restore_target_globals from
> +     arm_pragma_target_parse.  */
>
> Two spaces between fullstop and "#pragma GCC".

thanks,

Christian


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

2016-01-20  Christian Bruel  <christian.bruel@st.com>

	PR target/69245
	* config/arm/arm-c.c (arm_pragma_target_parse): Add comments.
	Move arm_reset_previous_fndecl and set_target_option_current_node in
	the conditional part. Call save_restore_target_globals.
	* config/arm/arm.c (arm_set_current_function):
	Refactor to better support #pragma target and attribute mix.
	Call save_restore_target_globals.
	* config/arm/arm-protos.h (save_restore_target_globals): New function.

2016-01-20  Christian Bruel  <christian.bruel@st.com>

	PR target/69245
	* gcc.target/arm/pr69245.c: New test.

Index: gcc/config/arm/arm-c.c
===================================================================
--- gcc/config/arm/arm-c.c	(revision 232831)
+++ gcc/config/arm/arm-c.c	(working copy)
@@ -221,9 +221,6 @@ arm_pragma_target_parse (tree args, tree pop_targe
 	}
     }
 
-  target_option_current_node = cur_tree;
-  arm_reset_previous_fndecl ();
-
   /* Figure out the previous mode.  */
   prev_opt  = TREE_TARGET_OPTION (prev_tree);
   cur_opt   = TREE_TARGET_OPTION (cur_tree);
@@ -259,6 +256,18 @@ arm_pragma_target_parse (tree args, tree pop_targe
       arm_cpu_builtins (parse_in);
 
       cpp_opts->warn_unused_macros = saved_warn_unused_macros;
+
+      /* Make sure that target_reinit is called for next function, since
+	 TREE_TARGET_OPTION might change with the #pragma even if there is
+	 no target attribute attached to the function.  */
+      arm_reset_previous_fndecl ();
+
+      /* If going to the default mode, we restore the initial states.
+	 if cur_tree is a new target, states will be saved/restored on a per
+	 function basis in arm_set_current_function.  */
+      if (cur_tree == target_option_default_node)
+	save_restore_target_globals (cur_tree);
+
     }
 
   return true;
Index: gcc/config/arm/arm-protos.h
===================================================================
--- gcc/config/arm/arm-protos.h	(revision 232831)
+++ gcc/config/arm/arm-protos.h	(working copy)
@@ -332,6 +332,7 @@ extern bool arm_autoinc_modes_ok_p (machine_mode,
 extern void arm_emit_eabi_attribute (const char *, int, int);
 
 extern void arm_reset_previous_fndecl (void);
+extern void save_restore_target_globals (tree);
 
 /* Defined in gcc/common/config/arm-common.c.  */
 extern const char *arm_rewrite_selected_cpu (const char *name);
Index: gcc/config/arm/arm.c
===================================================================
--- gcc/config/arm/arm.c	(revision 232831)
+++ gcc/config/arm/arm.c	(working copy)
@@ -3446,8 +3446,7 @@ arm_option_override (void)
 
   /* Save the initial options in case the user does function specific
      options.  */
-  target_option_default_node = target_option_current_node
-    = build_target_option_node (&global_options);
+  target_option_default_node = build_target_option_node (&global_options);
 
   /* Init initial mode for testing.  */
   thumb_flipper = TARGET_THUMB;
@@ -29746,7 +29745,27 @@ arm_is_constant_pool_ref (rtx x)
 /* Remember the last target of arm_set_current_function.  */
 static GTY(()) tree arm_previous_fndecl;
 
+/* Restore or save the TREE_TARGET_GLOBALS from or to NEW_TREE.  */
+
+void
+save_restore_target_globals (tree new_tree)
+{
+  /* If we have a previous state, use it.  */
+  if (TREE_TARGET_GLOBALS (new_tree))
+    restore_target_globals (TREE_TARGET_GLOBALS (new_tree));
+  else if (new_tree == target_option_default_node)
+    restore_target_globals (&default_target_globals);
+  else
+    {
+      /* Call target_reinit and save the state for TARGET_GLOBALS.  */
+      TREE_TARGET_GLOBALS (new_tree) = save_target_globals_default_opts ();
+    }
+
+  arm_option_params_internal ();
+}
+
 /* Invalidate arm_previous_fndecl.  */
+
 void
 arm_reset_previous_fndecl (void)
 {
@@ -29756,6 +29775,7 @@ arm_reset_previous_fndecl (void)
 /* Establish appropriate back-end context for processing the function
    FNDECL.  The argument might be NULL to indicate processing at top
    level, outside of any function scope.  */
+
 static void
 arm_set_current_function (tree fndecl)
 {
@@ -29768,38 +29788,23 @@ arm_set_current_function (tree fndecl)
 
   tree new_tree = DECL_FUNCTION_SPECIFIC_TARGET (fndecl);
 
-  arm_previous_fndecl = fndecl;
+  /* If current function has no attributes but previous one did,
+     use the default node."  */
+  if (! new_tree && old_tree)
+    new_tree = target_option_default_node;
+
+  /* If nothing to do return.  #pragma GCC reset or #pragma GCC pop to
+     the default have been handled by save_restore_target_globals from
+     arm_pragma_target_parse.  */
   if (old_tree == new_tree)
     return;
 
-  if (new_tree && new_tree != target_option_default_node)
-    {
-      cl_target_option_restore (&global_options,
-				TREE_TARGET_OPTION (new_tree));
+  arm_previous_fndecl = fndecl;
 
-      if (TREE_TARGET_GLOBALS (new_tree))
-	restore_target_globals (TREE_TARGET_GLOBALS (new_tree));
-      else
-	TREE_TARGET_GLOBALS (new_tree)
-	  = save_target_globals_default_opts ();
-    }
+  /* First set the target options.  */
+  cl_target_option_restore (&global_options, TREE_TARGET_OPTION (new_tree));
 
-  else if (old_tree && old_tree != target_option_default_node)
-    {
-      new_tree = target_option_current_node;
-
-      cl_target_option_restore (&global_options,
-				TREE_TARGET_OPTION (new_tree));
-      if (TREE_TARGET_GLOBALS (new_tree))
-	restore_target_globals (TREE_TARGET_GLOBALS (new_tree));
-      else if (new_tree == target_option_default_node)
-	restore_target_globals (&default_target_globals);
-      else
-	TREE_TARGET_GLOBALS (new_tree)
-	  = save_target_globals_default_opts ();
-    }
-
-  arm_option_params_internal ();
+  save_restore_target_globals (new_tree);
 }
 
 /* Implement TARGET_OPTION_PRINT.  */
Index: gcc/testsuite/gcc.target/arm/pr69245.c
===================================================================
--- gcc/testsuite/gcc.target/arm/pr69245.c	(revision 0)
+++ gcc/testsuite/gcc.target/arm/pr69245.c	(working copy)
@@ -0,0 +1,26 @@
+/* PR target/69245 */
+/* Test that pop_options restores the vfp fpu mode.  */
+/* { dg-do compile } */
+/* { dg-require-effective-target arm_neon_ok } */
+/* { dg-require-effective-target arm_fp_ok } */
+/* { dg-options "-O2" } */
+/* { dg-add-options arm_fp } */
+
+#pragma GCC target "fpu=vfp"
+
+#pragma GCC push_options
+#pragma GCC target "fpu=neon-vfpv4"
+int a, c, d;
+float b;
+static int fn1 ()
+{
+  return 0;
+}
+#pragma GCC pop_options
+
+void fn2 ()
+{
+  d = b * c + a;
+}
+
+/* { dg-final { scan-assembler-times "\.fpu vfp" 1 } } */

Re: [PATCH][ARM] Fix PR target/69245 Rewrite arm_set_current_function

[Kyrill Tkachov]

On 26/01/16 16:56, Christian Bruel wrote:
>
>
> On 01/26/2016 04:58 PM, Kyrill Tkachov wrote:
>> Hi Christian,
>>
>> On 26/01/16 15:29, Christian Bruel wrote:
>>>
>>> On 01/25/2016 05:37 PM, Kyrill Tkachov wrote:
>>>
>>>
>>> So this is ok for trunk with the testcase changed as discussed above and using -O2
>>> optimisation level and with a couple comment fixes below.
>>>
>>> Hi Kyrill,
>>>
>>> I realized afterwards that my implementation had a flaw with the handling of #pragma GCC reset. What happened is that when both old and new TREE_TARGET_OPTION are NULL, we didn't save_restore the globals flags, to save compute time. The
>>> problem is that with #pragma GCC reset, we also fall into this situation, and exit without calling target_reeinit :-(
>>>
>>> Handling this situation doesn't complicate the code much, because I factorized the calls to target_reeinit + restore_target_globals into a new function (save_restore_target_globals). This function is called from the pragma context when
>>> resetting the state arm_reset_previous_fndecl to the default, and from arm_set_current_function when setting to a new target. This is only done when there is a change of the target flags, so no extra computing cost.
>>>
>>> Same testing as with previous patch:
>>>      arm-qemu/
>>>      arm-qemu//-mfpu=neon-fp-armv8
>>>      arm-qemu//-mfpu=neon
>>>
>>> Still OK ?
>>>
>> +/* Restore the TREE_TARGET_GLOBALS from previous state, or save it.  */
>> +static void
>> +save_restore_target_globals (tree new_tree)
>> +{
>> +  /* If we have a previous state, use it.  */
>> +  if (TREE_TARGET_GLOBALS (new_tree))
>> +    restore_target_globals (TREE_TARGET_GLOBALS (new_tree));
>> +  else if (new_tree == target_option_default_node)
>> +    restore_target_globals (&default_target_globals);
>> +  else
>> +    {
>> +      /* Call target_reinit and save the state for TARGET_GLOBALS.  */
>> +      TREE_TARGET_GLOBALS (new_tree) = save_target_globals_default_opts ();
>> +    }
>> +
>> +  arm_option_params_internal ();
>> +}
>>
>> Space before the function comment and signature.
> what do you mean ?  a new line between the comment and the function signature ? I usually mimic what's done in the other arm.c declarations, which sometimes have a new line, sometime not. It's not clear to me what's mandatory here, even 
> in the other parts of the compiler.
>

Yes, I meant new line, sorry. A new line is the rule, though there are some
functions that don't follow it. I guess we can fix those as we encounter them.

>>   Also, you need to document what is the NEW_TREE
>> parameter.
>>
>>    /* Invalidate arm_previous_fndecl.  */
>>    void
>> -arm_reset_previous_fndecl (void)
>> +arm_reset_previous_fndecl (tree new_tree)
>>    {
>> +  if (new_tree)
>> +    save_restore_target_globals (new_tree);
>> +
>>      arm_previous_fndecl = NULL_TREE;
>>    }
>>
>> I'm a bit wary of complicating this function. Suddenly it doesn't just reset the previous fndecl
>> but also restores globals and can save stuff into its argument. It's suddenly not clear what it's
>> purpose is.
>> I think it would be clearer if you just added save_restore_target_globals to arm_protos.h and called
>> it explicitly from arm_pragma_target_parse when appropriate.
>
> sure, like this one attached (sanity checked) ?
>
>

This is ok if it passes a proper testing round.

Thanks,
Kyrill


>>
>> +
>> +  /* If nothing to do return. #pragma GCC reset or #pragma GCC pop to
>> +     the default have been handled by save_restore_target_globals from
>> +     arm_pragma_target_parse.  */
>>
>> Two spaces between fullstop and "#pragma GCC".
>
> thanks,
>
> Christian
>