| Message ID | 55E9644E.2080708@arm.com |
|---|---|
| State | New |
| Headers | show |
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}. >
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.