On Fri, May 28, 2021 at 11:48 AM Martin Liška <mli...@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?

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

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

> 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

Reply via email to