Re: [PATCH][ARM] Fix PR target/69245 Rewrite arm_set_current_function

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

Hi Christian,

On 26/01/16 15:29, Christian Bruel wrote:
> On 01/25/2016 05:37 PM, Kyrill Tkachov wrote:
>
>
> So this is ok for trunk with the testcase changed as discussed above and using -O2
> optimisation level and with a couple comment fixes below.
>
> Hi Kyrill,
>
> I realized afterwards that my implementation had a flaw with the handling of #pragma GCC reset. What happened is that when both old and new TREE_TARGET_OPTION are NULL, we didn't save_restore the globals flags, to save compute time. The 
> problem is that with #pragma GCC reset, we also fall into this situation, and exit without calling target_reeinit :-(
>
> Handling this situation doesn't complicate the code much, because I factorized the calls to target_reeinit + restore_target_globals into a new function (save_restore_target_globals). This function is called from the pragma context when 
> resetting the state arm_reset_previous_fndecl to the default, and from arm_set_current_function when setting to a new target. This is only done when there is a change of the target flags, so no extra computing cost.
>
> Same testing as with previous patch:
>     arm-qemu/
>     arm-qemu//-mfpu=neon-fp-armv8
>     arm-qemu//-mfpu=neon
>
> Still OK ?

+/* Restore the TREE_TARGET_GLOBALS from previous state, or save it.  */
+static void
+save_restore_target_globals (tree new_tree)
+{
+  /* If we have a previous state, use it.  */
+  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
+    {
+      /* Call target_reinit and save the state for TARGET_GLOBALS.  */
+      TREE_TARGET_GLOBALS (new_tree) = save_target_globals_default_opts ();
+    }
+
+  arm_option_params_internal ();
+}

Space before the function comment and signature. Also, you need to document what is the NEW_TREE
parameter.

 /* Invalidate arm_previous_fndecl.  */
 void
-arm_reset_previous_fndecl (void)
+arm_reset_previous_fndecl (tree new_tree)
 {
+  if (new_tree)
+    save_restore_target_globals (new_tree);
+
   arm_previous_fndecl = NULL_TREE;
 }

I'm a bit wary of complicating this function. Suddenly it doesn't just reset the previous fndecl
but also restores globals and can save stuff into its argument. It's suddenly not clear what it's
purpose is.
I think it would be clearer if you just added save_restore_target_globals to arm_protos.h and called
it explicitly from arm_pragma_target_parse when appropriate.

+
+  /* If nothing to do return. #pragma GCC reset or #pragma GCC pop to
+     the default have been handled by save_restore_target_globals from
+     arm_pragma_target_parse.  */

Two spaces between fullstop and "#pragma GCC".

Thanks,
Kyrill