[PATCH, ARM]: Add TARGET_OPTION[RESTORE,SAVE,PRINT] hooks

[PATCH, ARM]: Add TARGET_OPTION[RESTORE,SAVE,PRINT] hooks

[Christian Bruel]

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

Hi,

This patch uses TARGET_OPTION_RESTORE and SAVE to switch the attribute 
target dependent params between functions.

This is more efficient than arm_option_params_internal, and prepares the 
ground for the other machine target attributes.

No regressions. OK for trunk ?

many thanks

Christian


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

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

	PR target/52144
	* config/arm/arm.c (TARGET_OPTION_PRINT, TARGET_OPTION_SAVE)
	(TARGET_OPTION_RESTORE): New hooks defined with...
	(arm_option_print, arm_function_specific_save)
	(arm_function_specific_restore) New functions.
	(arm_set_current_function): Move arm_option_params_internal call...
	(arm_valid_target_attribute_tree): here.
	* config/arm/arm.opt (min_anchor_offset, max_anchor_offset)
	(max_insns_skipped): New TargetSave variables.

Index: gcc/config/arm/arm.c
===================================================================
--- gcc/config/arm/arm.c	(revision 227366)
+++ gcc/config/arm/arm.c	(working copy)
@@ -245,9 +245,14 @@
 static void arm_expand_builtin_va_start (tree, rtx);
 static tree arm_gimplify_va_arg_expr (tree, tree, gimple_seq *, gimple_seq *);
 static void arm_option_override (void);
+static void arm_option_print (FILE *, int, struct cl_target_option *);
 static void arm_set_current_function (tree);
 static bool arm_can_inline_p (tree, tree);
 static bool arm_valid_target_attribute_p (tree, tree, tree, int);
+static void arm_function_specific_save (struct cl_target_option *,
+					struct gcc_options *);
+static void arm_function_specific_restore (struct gcc_options *,
+					   struct cl_target_option *);
 static unsigned HOST_WIDE_INT arm_shift_truncation_mask (machine_mode);
 static bool arm_macro_fusion_p (void);
 static bool arm_cannot_copy_insn_p (rtx_insn *);
@@ -405,6 +410,9 @@
 #undef  TARGET_OPTION_OVERRIDE
 #define TARGET_OPTION_OVERRIDE arm_option_override
 
+#undef TARGET_OPTION_PRINT
+#define TARGET_OPTION_PRINT arm_option_print
+
 #undef  TARGET_COMP_TYPE_ATTRIBUTES
 #define TARGET_COMP_TYPE_ATTRIBUTES arm_comp_type_attributes
 
@@ -426,6 +434,12 @@
 #undef TARGET_OPTION_VALID_ATTRIBUTE_P
 #define TARGET_OPTION_VALID_ATTRIBUTE_P arm_valid_target_attribute_p
 
+#undef TARGET_OPTION_SAVE
+#define TARGET_OPTION_SAVE arm_function_specific_save
+
+#undef TARGET_OPTION_RESTORE
+#define TARGET_OPTION_RESTORE arm_function_specific_restore
+
 #undef TARGET_SCHED_REORDER
 #define TARGET_SCHED_REORDER arm_sched_reorder
 
@@ -29462,8 +29476,37 @@
 	TREE_TARGET_GLOBALS (new_tree)
 	  = save_target_globals_default_opts ();
     }
+}
 
-  arm_option_params_internal (&global_options);
+/* TARGET_OPTION_PRINT hook.  */
+
+static void
+arm_option_print (FILE *file, int indent, struct cl_target_option *ptr)
+{
+  int flags = ptr->x_target_flags;
+
+  if (TARGET_THUMB_P (flags))
+    fprintf (file, "%*sselected thumb arch \n", indent, "");
+  else
+    fprintf (file, "%*sselected arm arch\n", indent, "");
+}
+
+static void
+arm_function_specific_save (struct cl_target_option *ptr,
+			    struct gcc_options *opts)
+{
+  ptr->min_anchor_offset = targetm.min_anchor_offset;
+  ptr->max_anchor_offset = targetm.max_anchor_offset;
+  ptr->max_insns_skipped = max_insns_skipped;
+}
+
+static void
+arm_function_specific_restore (struct gcc_options *opts,
+			       struct cl_target_option *ptr)
+{
+  targetm.min_anchor_offset = ptr->min_anchor_offset;
+  targetm.max_anchor_offset = ptr->max_anchor_offset;
+  max_insns_skipped = ptr->max_insns_skipped;
 }
 
 /* Hook to determine if one function can safely inline another.  */
@@ -29538,6 +29581,7 @@
 
   /* Do any overrides, such as global options arch=xxx.  */
   arm_option_override_internal (opts, opts_set);
+  arm_option_params_internal (opts);
 
   return build_target_option_node (opts);
 }
Index: gcc/config/arm/arm.opt
===================================================================
--- gcc/config/arm/arm.opt	(revision 227366)
+++ gcc/config/arm/arm.opt	(working copy)
@@ -281,3 +281,13 @@
 masm-syntax-unified
 Target Report Var(inline_asm_unified) Init(0) Save
 Assume unified syntax for Thumb inline assembly code.
+
+;; Definitions to add to the cl_target_option structure
+TargetSave
+HOST_WIDE_INT min_anchor_offset
+
+TargetSave
+HOST_WIDE_INT max_anchor_offset
+
+TargetSave
+int max_insns_skipped

Re: [PATCH, ARM]: Add TARGET_OPTION[RESTORE,SAVE,PRINT] hooks

[Kyrill Tkachov]

Hi Christian,

Thanks again for working on this!

On 02/09/15 10:11, Christian Bruel wrote:
> Hi,
>
> This patch uses TARGET_OPTION_RESTORE and SAVE to switch the attribute
> target dependent params between functions.
>
> This is more efficient than arm_option_params_internal, and prepares the
> ground for the other machine target attributes.

To be honest, I'm reluctant to take this approach.
The design I'd like to see is for us to figure out the minimal set
of TargetSave variables (and options in arm.opt that require the tag Save)
and use them to 'package' the backend state whenever needed, with the help
of TARGET_OPTION_SAVE when appropriate.

Then during TARGET_OPTION_RESTORE we would call arm_option_override_internal
and arm_option_params_internal to restore all the different backend
variables that we need.

In my opinion that is the approach that would give the most maintainability.
In any case, I see arm_option_params_internal is not particularly complicated
at the moment, so I don't think your patch would show any noticeable speed improvement
(unless you can share data to demonstrate that :))

Maybe we can make the roadmap for this more concrete if you post a description of
what parameters you'd like to save and restore to achieve this goal and we can discuss
that. Some that come to mind would be: CPU we're tuning for, architecture, FPU, and
basically any option in arm.opt which we want to allow to vary on a per-function basis
(i.e. no ABI-changing options).

Hope this helps,

Kyrill

P.S.
I recently implemented this functionality in the aarch64 backend.
Maybe the implementation of these hooks over there could be of some help
(though I appreciate the arm backend is more complicated with more legacy and option variations)

> No regressions. OK for trunk ?
>
> many thanks
>
> Christian
>

Index: gcc/config/arm/arm.c
===================================================================
--- gcc/config/arm/arm.c	(revision 227366)
+++ gcc/config/arm/arm.c	(working copy)
@@ -245,9 +245,14 @@
  static void arm_expand_builtin_va_start (tree, rtx);
  static tree arm_gimplify_va_arg_expr (tree, tree, gimple_seq *, gimple_seq *);
  static void arm_option_override (void);
+static void arm_option_print (FILE *, int, struct cl_target_option *);
  static void arm_set_current_function (tree);
  static bool arm_can_inline_p (tree, tree);
  static bool arm_valid_target_attribute_p (tree, tree, tree, int);
+static void arm_function_specific_save (struct cl_target_option *,
+					struct gcc_options *);
+static void arm_function_specific_restore (struct gcc_options *,
+					   struct cl_target_option *);


I'd rather call them arm_option_save and arm_option_restore.
That way it's easy to deduce that they implement TARGET_OPTION_{SAVE,RESTORE}.

Re: [PATCH, ARM]: Add TARGET_OPTION[RESTORE,SAVE,PRINT] hooks

[Christian Bruel]

Hi Kyrill,

On 09/04/2015 11:28 AM, Kyrill Tkachov wrote:
> Hi Christian,
>
> Thanks again for working on this!
>
> On 02/09/15 10:11, Christian Bruel wrote:
>> Hi,
>>
>> This patch uses TARGET_OPTION_RESTORE and SAVE to switch the attribute
>> target dependent params between functions.
>>
>> This is more efficient than arm_option_params_internal, and prepares the
>> ground for the other machine target attributes.
>
> To be honest, I'm reluctant to take this approach.
> The design I'd like to see is for us to figure out the minimal set
> of TargetSave variables (and options in arm.opt that require the tag Save)
> and use them to 'package' the backend state whenever needed, with the help
> of TARGET_OPTION_SAVE when appropriate.
>
> Then during TARGET_OPTION_RESTORE we would call arm_option_override_internal
> and arm_option_params_internal to restore all the different backend
> variables that we need.
>
> In my opinion that is the approach that would give the most maintainability.
> In any case, I see arm_option_params_internal is not particularly complicated
> at the moment, so I don't think your patch would show any noticeable speed improvement
> (unless you can share data to demonstrate that :))
>

yes, both approaches are equally not-complex. Calling 
arm_option_params_internal instead of saving/restoring variables costs a 
couple of if-then-elses, I don't think the gain can be measurable with 
the hooks indeed.
I wrongly thought the hook could justify the saving, but note that 
maintainability is not in sake because arm_option_override_internal is 
still the central point for the initial settings.

> Maybe we can make the roadmap for this more concrete if you post a description of
> what parameters you'd like to save and restore to achieve this goal and we can discuss
> that. Some that come to mind would be: CPU we're tuning for, architecture, FPU, and
> basically any option in arm.opt which we want to allow to vary on a per-function basis
> (i.e. no ABI-changing options).

For now I only expect to support -mfpu. And for this one I only need to 
save arm_fpu_index, since the FPU params are indexed by it.

So OK lets keep calling arm_option_params_internal without new 
cl_target_option fields.

Potentially all the other -moptions that don't create link-time 
conflicts will be candidate, looking at arm.opt there are not so many. 
Tuning options like -mrestrict-it or other neon (mneon-for-64bits) would 
be more important and certainly come first in the list, but now that the 
infra is there that should be quite fast.

Best Regards

Christian

>
> Hope this helps,
>
> Kyrill
>
> P.S.
> I recently implemented this functionality in the aarch64 backend.
> Maybe the implementation of these hooks over there could be of some help
> (though I appreciate the arm backend is more complicated with more legacy and option variations)
>
>> No regressions. OK for trunk ?
>>
>> many thanks
>>
>> Christian
>>
>
> Index: gcc/config/arm/arm.c
> ===================================================================
> --- gcc/config/arm/arm.c	(revision 227366)
> +++ gcc/config/arm/arm.c	(working copy)
> @@ -245,9 +245,14 @@
>    static void arm_expand_builtin_va_start (tree, rtx);
>    static tree arm_gimplify_va_arg_expr (tree, tree, gimple_seq *, gimple_seq *);
>    static void arm_option_override (void);
> +static void arm_option_print (FILE *, int, struct cl_target_option *);
>    static void arm_set_current_function (tree);
>    static bool arm_can_inline_p (tree, tree);
>    static bool arm_valid_target_attribute_p (tree, tree, tree, int);
> +static void arm_function_specific_save (struct cl_target_option *,
> +					struct gcc_options *);
> +static void arm_function_specific_restore (struct gcc_options *,
> +					   struct cl_target_option *);
>
>
> I'd rather call them arm_option_save and arm_option_restore.
> That way it's easy to deduce that they implement TARGET_OPTION_{SAVE,RESTORE}.


>