On Tue, Jun 1, 2021 at 1:17 PM Martin Liška <mli...@suse.cz> wrote:
>
> On 5/28/21 2:46 PM, Richard Biener wrote:
> > 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?
>
> 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?
It sounds like a clear progression so OK.
I still don't get
+ /* When #pragma GCC optimize pragma is used, it modifies global_options
+ without calling targetm.override_options_after_change. That can leave
+ target flags inconsistent for comparison. */
fully, esp. as to why we cannot fix pragma handling and thus why the
"inconsistent"
state is actually OK.
Richard.
> 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
>
