Fallout: save/restore target options in handle_optimize_attribute

Tue Jun 1 11:17:51 GMT 2021

On 5/28/21 2:46 PM, Richard Biener wrote:
> On Fri, May 28, 2021 at 11:48 AM Martin Liška <mliska@suse.cz> wrote:
>>
>> Hi.
>>
>> There's a fallout after my revision ebd5e86c0f41dc1d692f9b2b68a510b1f6835a3e. I would like to analyze
>> all case and discuss possible solution. To be honest it's a can of worms and reverting the commit
>> is an option on the table.
>>
>> So the cases:
>>
>> 1) PR100759 - ppc64le
>>
>> $ cat pr.C
>> #pragma GCC optimize 0
>> void main();
>>
>> $ ./xgcc -B. -Os pr.C
>> pr.C:2:11: internal compiler error: ‘global_options’ are modified in local context
>>       2 | void main();
>>
>> What happens: we change from -Os to -O0 and rs6000_isa_flags differ in cl_optimization_compare.
>> Problem is that OPTION_MASK_SAVE_TOC_INDIRECT is set based on optimize flag:
>>
>>     /* If we can shrink-wrap the TOC register save separately, then use
>>        -msave-toc-indirect unless explicitly disabled.  */
>>     if ((rs6000_isa_flags_explicit & OPTION_MASK_SAVE_TOC_INDIRECT) == 0
>>         && flag_shrink_wrap_separate
>>         && optimize_function_for_speed_p (cfun))
>>       rs6000_isa_flags |= OPTION_MASK_SAVE_TOC_INDIRECT;
> 
> So that means that
> 
>        /* Restore current options.  */
>        cl_optimization_restore (&global_options, &global_options_set,
>                                 &cur_opts);
>        cl_target_option_restore (&global_options, &global_options_set,
>                                  TREE_TARGET_OPTION (prev_target_node));
> 
> does not result in the same outcome as the original command-line processing?
> 
> Given both restore processes could interact (not sure if that's the issue here)
> shouldn't we just have a single restore operation and a single target
> hook instead of both targetm.override_options_after_change and
> targetm.target_option.restore?

That's not this case. But it can be a unification approach for the future.

> 
> Likewise we should probably _always_ set both, DECL_FUNCTION_SPECIFIC_OPT
> and _TARGET as a step towards unifying them.

Yes, that's basically what's happening at various places.

> 
> That said, for the above case a more detailed run-down as to how things go wrong
> would be nice to see.

Anyway, detail analysis of this issue is:

1) one provides -Os on the command-line, thus global_options.x_optimize_size == 1
2) then we reach #pragma GCC optimize 0, at this point parse_optimize_options is called
    and thus global_options are modified (global_options.x_optimize_size)
    That's reflected in optimization_current_node, which is now different from optimization_default_node.
3) targetm.override_options_after_change is not called, so global_options.x_rs6000_isa_flags
    is not changed to 1.
4) for all subsequent functions, handle_optimize_attribute is called as we are in a 'pragma optimize'
5) here the sanity checking code saves saved_global_options, parsing happens and cl_*_restore is done
6) as cl_target_option_restore calls targetm.override_options_after_change, the global_options.x_rs6000_isa_flags
    has OPTION_MASK_SAVE_TOC_INDIRECT set
7) and the cl_optimization_compare complains

I have a patch that reflects that. In fact, we global options state is correct for each function.
Apart from that, PR100759 mentions a test-case that fails due to a missing cl_target_option_restore
for 'pragma pop'.

Patch can bootstrap on x86_64-linux-gnu and survives regression tests. And it survives tests on ppc64-linux-gnu.

Ready to be installed?
Thanks,
Martin

> 
>> Suggested solution is doing:
>>
>>     if ((rs6000_isa_flags_explicit & OPTION_MASK_SAVE_TOC_INDIRECT) == 0
>>         && flag_shrink_wrap_separate
>>       rs6000_isa_flags |= OPTION_MASK_SAVE_TOC_INDIRECT;
>>
>> and add '&& optimize_function_for_speed_p (cfun)' to the place where the option mask is used.
>>
>> 2) Joseph's case:
>>
>> $ cat ~/Programming/testcases/opts-bug.i
>> extern unsigned long int x;
>> extern float f (float);
>> extern __typeof (f) f_power8;
>> extern __typeof (f) f_power9;
>> extern __typeof (f) f __attribute__ ((ifunc ("f_ifunc")));
>> static __attribute__ ((optimize ("-fno-stack-protector"))) __typeof (f) *
>> f_ifunc (void)
>> {
>>     __typeof (f) *res = x ? f_power9 : f_power8;
>>     return res;
>> }
>>
>> $ ./xgcc -B. ~/Programming/testcases/opts-bug.i -c -S -O2 -mlong-double-128 -mabi=ibmlongdouble
>> /home/marxin/Programming/testcases/opts-bug.i:8:1: error: ‘-mabi=ibmlongdouble’ requires ‘-mlong-double-128’
>>
>> This is caused by a weird option override:
>>
>>     else if (rs6000_long_double_type_size == 128)
>>       rs6000_long_double_type_size = FLOAT_PRECISION_TFmode; (it's 127)
>>
>> later when rs6000_option_override_internal is called for saved target flags (127), it complains.
>> Possible fix:
>>
>>     else if (rs6000_long_double_type_size == 128
>>             || rs6000_long_double_type_size == FLOAT_PRECISION_TFmode)
>>
>> 3) ARM issue reported here:
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98636#c20
>>
>>     arm_fp16_inst = bitmap_bit_p (arm_active_target.isa, isa_bit_fp16);
>>     if (arm_fp16_inst)
>>       {
>>         if (arm_fp16_format == ARM_FP16_FORMAT_ALTERNATIVE)
>>          error ("selected fp16 options are incompatible");
>>         arm_fp16_format = ARM_FP16_FORMAT_IEEE;
>>       }
>>
>> there's likely missing else branch which would reset when arm_fp16_inst is null.
>> Anyway, can be moved again to the ignored list
>>
>> 4) Jeff reported the following for v850-elf:
>>
>> $ cat ~/Programming/testcases/j.c
>> typedef __SIZE_TYPE__ size_t;
>>
>> extern inline __attribute__ ((__always_inline__, __gnu_inline__, __artificial__, __nothrow__, __leaf__)) void *
>> memcpy (void *__restrict __dest, const void *__restrict __src, size_t __len)
>> {
>>     return __builtin___memcpy_chk (__dest, __src, __len, __builtin_object_size (__dest, 0));
>> }
>>
>> __attribute__((optimize ("Ofast"))) void
>> bar (void *d, void *s, size_t l)
>> {
>>     memcpy (d, s, l);
>> }
>>
>> $ ./xgcc -B. ~/Programming/testcases/j.c  -S
>> /home/marxin/Programming/testcases/j.c: In function ‘bar’:
>> /home/marxin/Programming/testcases/j.c:4:1: error: inlining failed in call to ‘always_inline’ ‘memcpy’: target specific option mismatch
>>       4 | memcpy (void *__restrict __dest, const void *__restrict __src, size_t __len)
>>         | ^~~~~~
>> /home/marxin/Programming/testcases/j.c:12:3: note: called from here
>>      12 |   memcpy (d, s, l);
>>         |   ^~~~~~~~~~~~~~~~
>>
>> This one is pretty clear. The target does:
>>
>>       { OPT_LEVELS_1_PLUS, OPT_mprolog_function, NULL, 1 },
>>
>> So it sets a target option based on optimize level.
>> This one will need:
>>
>> diff --git a/gcc/config/v850/v850.c b/gcc/config/v850/v850.c
>> index e0e5005d865..49f91f12766 100644
>> --- a/gcc/config/v850/v850.c
>> +++ b/gcc/config/v850/v850.c
>> @@ -3140,6 +3140,11 @@ v850_option_override (void)
>>      /* The RH850 ABI does not (currently) support the use of the CALLT instruction.  */
>>      if (! TARGET_GCC_ABI)
>>        target_flags |= MASK_DISABLE_CALLT;
>> +
>> +  /* Save the initial options in case the user does function specific
>> +     options.  */
>> +  target_option_default_node = target_option_current_node
>> +    = build_target_option_node (&global_options, &global_options_set);
>>    }
>>
>> plus a custom can_inline_p target hook where the MASK_PROLOG_FUNCTION is ignored because
>> caller does not have it set, while callee has.
>>
>> What target maintainers thing about it?
>>
>> Martin

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Fix-sanity-checking-of-global_options.patch
Type: text/x-patch
Size: 1729 bytes
Desc: not available
URL: <https://gcc.gnu.org/pipermail/gcc-patches/attachments/20210601/01c84110/attachment.bin>