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

[Kyrill Tkachov <kyrylo dot tkachov at arm dot com>]

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}.