Fallout: save/restore target options in handle_optimize_attribute

Message ID 74b71e01-ea81-9506-6756-6a1160878d45@suse.cz
State New
Headers show
Series
  • Fallout: save/restore target options in handle_optimize_attribute
Related show

Commit Message

Martin Liška May 28, 2021, 9:48 a.m.
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

Comments

H.J. Lu via Gcc-patches May 28, 2021, 12:46 p.m. | #1
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
Segher Boessenkool May 28, 2021, 1:35 p.m. | #2
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
Martin Liška June 1, 2021, 11:17 a.m. | #3
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
From 2653f4d931ec124cf24c13ecb980db83ad000ce1 Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>

Date: Tue, 1 Jun 2021 10:41:04 +0200
Subject: [PATCH] Fix sanity checking of global_options.

---
 gcc/c-family/c-attribs.c | 6 +++++-
 gcc/c-family/c-pragma.c  | 2 ++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
index 804374d5acc..156f7b3e8e1 100644
--- a/gcc/c-family/c-attribs.c
+++ b/gcc/c-family/c-attribs.c
@@ -5389,7 +5389,11 @@ handle_optimize_attribute (tree *node, tree name, tree args,
       /* If we previously had some optimization options, use them as the
 	 default.  */
       gcc_options *saved_global_options = NULL;
-      if (flag_checking)
+
+      /* 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.  */
+      if (flag_checking && optimization_current_node == optimization_default_node)
 	{
 	  saved_global_options = XNEW (gcc_options);
 	  *saved_global_options = global_options;
diff --git a/gcc/c-family/c-pragma.c b/gcc/c-family/c-pragma.c
index 7f658ea5646..f46b5b93c29 100644
--- a/gcc/c-family/c-pragma.c
+++ b/gcc/c-family/c-pragma.c
@@ -1088,6 +1088,8 @@ handle_pragma_pop_options (cpp_reader *ARG_UNUSED(dummy))
    * overwritten by invoke_set_current_function_hook.  */
   cl_optimization_restore (&global_options, &global_options_set,
 			   TREE_OPTIMIZATION (p->optimize_binary));
+  cl_target_option_restore (&global_options, &global_options_set,
+			    TREE_TARGET_OPTION (p->target_binary));
 
   if (p->optimize_binary != optimization_current_node)
     {
-- 
2.31.1
H.J. Lu via Gcc-patches June 1, 2021, 1:11 p.m. | #4
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

>
Martin Liška June 1, 2021, 1:20 p.m. | #5
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

>>

Patch

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