| Message ID | 74b71e01-ea81-9506-6756-6a1160878d45@suse.cz |
|---|---|
| State | New |
| Headers | show |
| Series | Fallout: save/restore target options in handle_optimize_attribute | expand |
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? 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
Hi! On Fri, May 28, 2021 at 11:48:06AM +0200, Martin Liška wrote: > 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. > $ ./xgcc -B. -Os pr.C > pr.C:2:11: internal compiler error: ‘global_options’ are modified in > local context > 2 | void main(); So what is called "global" and "local" there? There are at least three levels (cannot modify within a TU, cannot modify within a function, and anything goes). What is this about? > 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; Why is that a problem? This has always been allowed, and disallowing it now will have a lot of fallout. Why change the basic features of the model at all, is there a very good reason for that? > 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) This is a workaround for the fact that GCC insists all floating point types can be ordered. They can not. There are numbers in both double-double ("IBM extended long double") and in IEEE QP float that cannot be represented with the same precision in the other format. So Mike made GCC think one format is "better" than the other depending on which we default to. This hack works remarkably well, but is of course a hack. Until the generic problems here are fixed we cannot do better. > 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) Please propose that as separate patch, and/or open a PR for it. Having everything in one mail thread might be easiest for you, but it isn't for everyone else ;-) > What target maintainers thing about it? That we need a lot more background: what do you want to do, and why? Segher
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
On Tue, Jun 1, 2021 at 1:17 PM Martin Liška <mliska@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 <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? 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 >
On 6/1/21 3:11 PM, Richard Biener wrote: > On Tue, Jun 1, 2021 at 1:17 PM Martin Liška <mliska@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 <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? > > It sounds like a clear progression so OK. Good, I'm going to install it. > > 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. Well, the sanity check is designed simply as it saved global_options, then parse_optimize_options happens and cl_*_restore is done. After that we want to be sure the global_options is equal to the saved one. And here comes the problem. We saved global_options modified after '#pragma GCC optimize 0'. Martin > > 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 >>
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
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; 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: caller does not have it set, while callee has. What target maintainers thing about it? Martin