[ARM] attribute target (thumb,arm) [4/6] respin (5th)

On 18/05/15 09:14, Christian Bruel wrote:
> Hi,

Hi Christian,
A couple comments inline.
Overall, the approach looks ok to me, though I think we'll have to
generalise arm_valid_target_attribute_rec in the future if we want
to allow other target attributes.

Thanks,
Kyrill

>
> Here is again a new version for patch [4/6]: (Implements and document
> the hooks to support target_attributes.)
>
>    - Rewrote for conflicts introduced by the new macros defined in
>      https://gcc.gnu.org/ml/gcc-patches/2015-05/msg01198.html
>
>    - Take your comments and Sandra's for documentation
>
>    - I'd like to leave the inlining question apart with its own patch
> until we settle it. Is it OK with you ?
>      https://gcc.gnu.org/ml/gcc-patches/2015-05/msg01527.html
>
> many thanks,
>
> Christian
>
> p4.patch
>
>
> 2014-09-23  Christian Bruel<christian.bruel@st.com>
>
> 	* config/arm/arm.opt (THUMB, arm_restrict_it, inline_asm_unified): Save.
> 	* config/arm/arm.h (arm_valid_target_attribute_tree): Declare.
> 	(arm_reset_previous_fndecl, arm_change_mode_p): Likewise.
> 	(SWITCHABLE_TARGET): Define.
> 	* config/arm/arm.c (arm_reset_previous_fndecl): New functions.
> 	(arm_valid_target_attribute_tree, arm_change_mode_p): Likewise.
> 	(arm_valid_target_attribute_p): Likewise.
> 	(arm_set_current_function, arm_can_inline_p): Likewise.
> 	(arm_valid_target_attribute_rec): Likewise.
> 	(arm_previous_fndecl): New variable.
> 	(TARGET_SET_CURRENT_FUNCTION, TARGET_OPTION_VALID_ATTRIBUTE_P): Define.
> 	(TARGET_CAN_INLINE_P): Define.
> 	(arm_asm_trampoline_template): Emit mode.
> 	(arm_file_start): Don't set unified syntax.
> 	(arm_declare_function_name): Set unified syntax and mode.
> 	(arm_option_override): Init target_option_default_node.
> 	and target_option_current_node.
> 	* config/arm/arm.md (*call_value_symbol): Set mode when possible.
> 	(*call_symbol): Likewise.
> 	* doc/extend.texi: Document ARM/Thumb target attribute.
> 	* doc/invoke.texi: Likewise.
>
> 2014-09-23  Christian Bruel<christian.bruel@st.com>
>
> 	* gcc.target/arm/attr_arm.c: New test
> 	* gcc.target/arm/attr_arm-err.c: New test
> 	* gcc.target/arm/attr_thumb.c: New test
> 	* gcc.target/arm/attr_thumb-static.c: New test

Full stops at end of "New test"

<snip>

  
+/* Check that FUNC is called with a different mode.  */
+
+bool
+arm_change_mode_p (tree func)
+{
+  if (TREE_CODE (func) != FUNCTION_DECL)
+    return false;
+
+  tree callee_tree = DECL_FUNCTION_SPECIFIC_TARGET (func);
+
+  if (!callee_tree)
+    callee_tree = target_option_default_node;
+
+  struct cl_target_option *callee_opts = TREE_TARGET_OPTION (callee_tree);
+  int flags = callee_opts ->x_target_flags;


No space after callee_opts.

<snip>
  
+/* Remember the last target of arm_set_current_function.  */
+static GTY(()) tree arm_previous_fndecl;
+
+/* Invalidate arm_previous_fndecl.  */
+void
+arm_reset_previous_fndecl (void)
+{
+  arm_previous_fndecl = NULL_TREE;
+}
+
+/* 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)
+{
+  if (!fndecl || fndecl == arm_previous_fndecl)
+    return;
+
+  tree old_tree = (arm_previous_fndecl
+		   ? DECL_FUNCTION_SPECIFIC_TARGET (arm_previous_fndecl)
+		   : NULL_TREE);
+
+  tree new_tree = DECL_FUNCTION_SPECIFIC_TARGET (fndecl);
+
+  arm_previous_fndecl = fndecl;
+  if (old_tree == new_tree)
+    ;
+
+  else if (new_tree)
+    {
+      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
+	TREE_TARGET_GLOBALS (new_tree)
+	  = save_target_globals_default_opts ();
+    }
+
+  else if (old_tree)
+    {
+      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 (&global_options);


I thought the more common approach was to define TARGET_OPTION_RESTORE
that was supposed to restore the backend state, including calling arm_option_params_internal?
That way, cl_target_option_restore would do all that needs to be done to restore the backend.

<snip>

> +{
> +  return a ? 1 : 5;
> +}
> +
> diff '--exclude=.svn' -ruN gnu_trunk.p2/gcc/gcc/testsuite/gcc.target/arm/attr_arm-err.c gnu_trunk.p4/gcc/gcc/testsuite/gcc.target/arm/attr_arm-err.c
> --- gnu_trunk.p2/gcc/gcc/testsuite/gcc.target/arm/attr_arm-err.c	1970-01-01 01:00:00.000000000 +0100
> +++ gnu_trunk.p4/gcc/gcc/testsuite/gcc.target/arm/attr_arm-err.c	2015-05-07 18:15:34.177443930 +0200
> @@ -0,0 +1,13 @@
> +/* Check that attribute target arm is rejected for M profile.  */
> +/* { dg-do compile } */
> +/* { dg-require-effective-target arm_arm_ok } */
> +/* { dg-skip-if "avoid conflicting multilib options" { *-*-* } { "-march=*" } { "-march=armv6-m" } } */
> +/* { dg-add-options arm_arch_v6m } */
> +
> +int __attribute__((target("arm")))
> +foo(int a)
> +{  /* { dg-error "does not support" } */
> +  return a ? 1 : 5;
> +}
> +
> +
> diff '--exclude=.svn' -ruN gnu_trunk.p2/gcc/gcc/testsuite/gcc.target/arm/attr_thumb.c gnu_trunk.p4/gcc/gcc/testsuite/gcc.target/arm/attr_thumb.c
> --- gnu_trunk.p2/gcc/gcc/testsuite/gcc.target/arm/attr_thumb.c	1970-01-01 01:00:00.000000000 +0100
> +++ gnu_trunk.p4/gcc/gcc/testsuite/gcc.target/arm/attr_thumb.c	2015-05-07 18:15:34.177443930 +0200
> @@ -0,0 +1,13 @@
> +/* Check that attribute target thumb is recogniwed. */
s/recogniwed/recognized/

hi Kyrill


On 06/01/2015 12:39 PM, Kyrill Tkachov wrote:
>
> On 18/05/15 09:14, Christian Bruel wrote:
>> Hi,
>
> Hi Christian,
> A couple comments inline.
> Overall, the approach looks ok to me, though I think we'll have to
> generalise arm_valid_target_attribute_rec in the future if we want
> to allow other target attributes.
>

the other fpu target attributes will be part of another set of 
developments, specific parsing strings will be added as they are 
implemented.

> +
> +/* 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)
> +{
> +  if (!fndecl || fndecl == arm_previous_fndecl)
> +    return;
> +
> +  tree old_tree = (arm_previous_fndecl
> +		   ? DECL_FUNCTION_SPECIFIC_TARGET (arm_previous_fndecl)
> +		   : NULL_TREE);
> +
> +  tree new_tree = DECL_FUNCTION_SPECIFIC_TARGET (fndecl);
> +
> +  arm_previous_fndecl = fndecl;
> +  if (old_tree == new_tree)
> +    ;
> +
> +  else if (new_tree)
> +    {
> +      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
> +	TREE_TARGET_GLOBALS (new_tree)
> +	  = save_target_globals_default_opts ();
> +    }
> +
> +  else if (old_tree)
> +    {
> +      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 (&global_options);
>
>
> I thought the more common approach was to define TARGET_OPTION_RESTORE
> that was supposed to restore the backend state, including calling arm_option_params_internal?
> That way, cl_target_option_restore would do all that needs to be done to restore the backend.
>

TARGET_OPTION_RESTORE is fine to restore target-specific
information from struct cl_target_option. Other global states might as 
well be expressed within set_current_function (e.g indeed I might use 
TARGET_OPTION_RESTORE to switch arm_fpu_attr in the fpu neon attribute). 
But IMHO arm_option_params_internal are fine to be called there since 
the 2 params only depend from x_target_flags without the need of a new 
macro.

On 01/06/15 12:29, Christian Bruel wrote:
> hi Kyrill
>
>
> On 06/01/2015 12:39 PM, Kyrill Tkachov wrote:
>> On 18/05/15 09:14, Christian Bruel wrote:
>>> Hi,
>> Hi Christian,
>> A couple comments inline.
>> Overall, the approach looks ok to me, though I think we'll have to
>> generalise arm_valid_target_attribute_rec in the future if we want
>> to allow other target attributes.
>>
> the other fpu target attributes will be part of another set of
> developments, specific parsing strings will be added as they are
> implemented.

Ok, so you plan on working on fpu attributes as well?

>
>> +
>> +/* 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)
>> +{
>> +  if (!fndecl || fndecl == arm_previous_fndecl)
>> +    return;
>> +
>> +  tree old_tree = (arm_previous_fndecl
>> +		   ? DECL_FUNCTION_SPECIFIC_TARGET (arm_previous_fndecl)
>> +		   : NULL_TREE);
>> +
>> +  tree new_tree = DECL_FUNCTION_SPECIFIC_TARGET (fndecl);
>> +
>> +  arm_previous_fndecl = fndecl;
>> +  if (old_tree == new_tree)
>> +    ;
>> +
>> +  else if (new_tree)
>> +    {
>> +      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
>> +	TREE_TARGET_GLOBALS (new_tree)
>> +	  = save_target_globals_default_opts ();
>> +    }
>> +
>> +  else if (old_tree)
>> +    {
>> +      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 (&global_options);
>>
>>
>> I thought the more common approach was to define TARGET_OPTION_RESTORE
>> that was supposed to restore the backend state, including calling arm_option_params_internal?
>> That way, cl_target_option_restore would do all that needs to be done to restore the backend.
>>
> TARGET_OPTION_RESTORE is fine to restore target-specific
> information from struct cl_target_option. Other global states might as
> well be expressed within set_current_function (e.g indeed I might use
> TARGET_OPTION_RESTORE to switch arm_fpu_attr in the fpu neon attribute).
> But IMHO arm_option_params_internal are fine to be called there since
> the 2 params only depend from x_target_flags without the need of a new
> macro.

Ok, I see.
The patch looks ok to me modulo the typo nits I pointed out, but I think Ramana
should have the final say here as he's already started reviewing it and it adds quite
a lot of functionality.

Thanks,
Kyrill

>
>

On 06/01/2015 02:41 PM, Kyrill Tkachov wrote:
>
> On 01/06/15 12:29, Christian Bruel wrote:
>> hi Kyrill
>>
>>
>> On 06/01/2015 12:39 PM, Kyrill Tkachov wrote:
>>> On 18/05/15 09:14, Christian Bruel wrote:
>>>> Hi,
>>> Hi Christian,
>>> A couple comments inline.
>>> Overall, the approach looks ok to me, though I think we'll have to
>>> generalise arm_valid_target_attribute_rec in the future if we want
>>> to allow other target attributes.
>>>
>> the other fpu target attributes will be part of another set of
>> developments, specific parsing strings will be added as they are
>> implemented.
>
> Ok, so you plan on working on fpu attributes as well?

I have a prototype for fpu=neon, it works locally but there are still a 
few corner cases and testing to sort out before sending a draft.

There are so many architectural variants to check that I might ask for 
help once it is a little bit more robust, and some might be similar with 
aarch64's simd.

>
>>
>>> +
>>> +/* 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)
>>> +{
>>> +  if (!fndecl || fndecl == arm_previous_fndecl)
>>> +    return;
>>> +
>>> +  tree old_tree = (arm_previous_fndecl
>>> +		   ? DECL_FUNCTION_SPECIFIC_TARGET (arm_previous_fndecl)
>>> +		   : NULL_TREE);
>>> +
>>> +  tree new_tree = DECL_FUNCTION_SPECIFIC_TARGET (fndecl);
>>> +
>>> +  arm_previous_fndecl = fndecl;
>>> +  if (old_tree == new_tree)
>>> +    ;
>>> +
>>> +  else if (new_tree)
>>> +    {
>>> +      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
>>> +	TREE_TARGET_GLOBALS (new_tree)
>>> +	  = save_target_globals_default_opts ();
>>> +    }
>>> +
>>> +  else if (old_tree)
>>> +    {
>>> +      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 (&global_options);
>>>
>>>
>>> I thought the more common approach was to define TARGET_OPTION_RESTORE
>>> that was supposed to restore the backend state, including calling arm_option_params_internal?
>>> That way, cl_target_option_restore would do all that needs to be done to restore the backend.
>>>
>> TARGET_OPTION_RESTORE is fine to restore target-specific
>> information from struct cl_target_option. Other global states might as
>> well be expressed within set_current_function (e.g indeed I might use
>> TARGET_OPTION_RESTORE to switch arm_fpu_attr in the fpu neon attribute).
>> But IMHO arm_option_params_internal are fine to be called there since
>> the 2 params only depend from x_target_flags without the need of a new
>> macro.
>
> Ok, I see.
> The patch looks ok to me modulo the typo nits I pointed out, but I think Ramana
> should have the final say here as he's already started reviewing it and it adds quite
> a lot of functionality.

OK thanks, I'll wait for Ramana's final words.

Cheers

Christian

>
> Thanks,
> Kyrill
>
>>
>>
>

Hi Ramana,

>
> Ok, I see.
> The patch looks ok to me modulo the typo nits I pointed out, but I think Ramana
> should have the final say here as he's already started reviewing it and it adds quite
> a lot of functionality.
>
> Thanks,
> Kyrill
>

do you have other feedbacks for the remaining parts ?

many thanks

Christian

On 08/06/15 09:45, Christian Bruel wrote:
> Hi Ramana,
>
>>
>> Ok, I see.
>> The patch looks ok to me modulo the typo nits I pointed out, but I
>> think Ramana
>> should have the final say here as he's already started reviewing it
>> and it adds quite
>> a lot of functionality.
>>
>> Thanks,
>> Kyrill
>>
>
> do you have other feedbacks for the remaining parts ?
>
> many thanks
>
> Christian


This is OK, thanks.

Ramana