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

[Kyrill Tkachov <kyrylo.tkachov@foss.arm.com>]

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
>