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
