On 26/01/16 16:56, Christian Bruel wrote:
On 01/26/2016 04:58 PM, Kyrill Tkachov wrote:
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.
what do you mean ? a new line between the comment and the function signature ? I usually mimic what's done in the other arm.c declarations, which sometimes have a new line, sometime not. It's not clear to me what's mandatory here, even
in the other parts of the compiler.
Yes, I meant new line, sorry. A new line is the rule, though there are some
functions that don't follow it. I guess we can fix those as we encounter them.
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.
sure, like this one attached (sanity checked) ?
This is ok if it passes a proper testing round.
Thanks,
Kyrill
+
+ /* 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,
Christian
