RFA: save/restore target options in handle_optimize_attribute

RFA: save/restore target options in handle_optimize_attribute

[Joern Wolfgang Rennecke]

[-- Attachment #1: Type: text/plain, Size: 636 bytes --]

We set default for some target options in TARGET_OPTION_OPTIMIZATION_TABLE,
but these can be overridden by specifying the corresponding explicit
-mXXX / -mno-XXX options.
When a function bears the attribue
__attribute__ ((optimize("02")))
the target options are set to the default for that optimization level,
which can be different from what was selected for the file as a whole.
As handle_optimize_attribute is right now, it will thus clobber the
target options, and with enable_checking it will then abort.

The attached patch makes it save and restore the target options.

Bootstrapped and regression tested on x86_64-pc-linux-gnu.

[-- Attachment #2: attr-targ-opt.txt --]
[-- Type: text/plain, Size: 1233 bytes --]

c-family/
        * handle_optimize_attribute: Save & restore target options too.

diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
index f54388e9939..82d85ef8e01 100644
--- a/gcc/c-family/c-attribs.c
+++ b/gcc/c-family/c-attribs.c
@@ -5333,10 +5333,13 @@ handle_optimize_attribute (tree *node, tree name, tree args,
   else
     {
       struct cl_optimization cur_opts;
+      struct cl_target_option cur_target_opts;
       tree old_opts = DECL_FUNCTION_SPECIFIC_OPTIMIZATION (*node);
 
       /* Save current options.  */
       cl_optimization_save (&cur_opts, &global_options, &global_options_set);
+      cl_target_option_save (&cur_target_opts, &global_options,
+			     &global_options_set);
 
       /* If we previously had some optimization options, use them as the
 	 default.  */
@@ -5359,6 +5362,8 @@ handle_optimize_attribute (tree *node, tree name, tree args,
       /* Restore current options.  */
       cl_optimization_restore (&global_options, &global_options_set,
 			       &cur_opts);
+      cl_target_option_restore (&global_options, &global_options_set,
+				&cur_target_opts);
       if (saved_global_options != NULL)
 	{
 	  cl_optimization_compare (saved_global_options, &global_options);

Re: RFA: save/restore target options in handle_optimize_attribute

[Richard Biener]

On Thu, May 20, 2021 at 12:29 AM Joern Wolfgang Rennecke
<joern.rennecke@riscy-ip.com> wrote:
>
> We set default for some target options in TARGET_OPTION_OPTIMIZATION_TABLE,
> but these can be overridden by specifying the corresponding explicit
> -mXXX / -mno-XXX options.
> When a function bears the attribue
> __attribute__ ((optimize("02")))
> the target options are set to the default for that optimization level,
> which can be different from what was selected for the file as a whole.
> As handle_optimize_attribute is right now, it will thus clobber the
> target options, and with enable_checking it will then abort.
>
> The attached patch makes it save and restore the target options.
>
> Bootstrapped and regression tested on x86_64-pc-linux-gnu.

That looks reasonable but of course it doesn't solve the issue that those
altered target options will not be in effect on the optimize("O2") function.

IIRC Martin has changes in the works to unify target & optimize here
which should obsolete this fix.  Martin - what's the state of this?  Do you
think this patch makes sense in the mean time (and maybe also on
the branch though the assert is not in effect there but the behavior
is still observed and unexpected).

Thanks,
Richard.

Re: RFA: save/restore target options in handle_optimize_attribute

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 1787 bytes --]

On 5/20/21 9:55 AM, Richard Biener wrote:
> On Thu, May 20, 2021 at 12:29 AM Joern Wolfgang Rennecke
> <joern.rennecke@riscy-ip.com> wrote:
>>
>> We set default for some target options in TARGET_OPTION_OPTIMIZATION_TABLE,
>> but these can be overridden by specifying the corresponding explicit
>> -mXXX / -mno-XXX options.
>> When a function bears the attribue
>> __attribute__ ((optimize("02")))
>> the target options are set to the default for that optimization level,
>> which can be different from what was selected for the file as a whole.
>> As handle_optimize_attribute is right now, it will thus clobber the
>> target options, and with enable_checking it will then abort.
>>
>> The attached patch makes it save and restore the target options.
>>
>> Bootstrapped and regression tested on x86_64-pc-linux-gnu.

Interesting, I prepared very similar patch for this stage1. My patch covers few more
cases where target options interfere with optimize options (and vice versa).

> 
> That looks reasonable but of course it doesn't solve the issue that those
> altered target options will not be in effect on the optimize("O2") function.
> 
> IIRC Martin has changes in the works to unify target & optimize here
> which should obsolete this fix.  Martin - what's the state of this?  Do you
> think this patch makes sense in the mean time (and maybe also on
> the branch though the assert is not in effect there but the behavior
> is still observed and unexpected).

Well, I really tried doing the merge but I failed. It's pretty huge task and I was
unable to get something reasonable for x86_64 target :/ However, my patch mitigates
2 more cases.

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

Ready to be installed?
Thanks,
Martin

> 
> Thanks,
> Richard.
> 


[-- Attachment #2: 0001-Improve-global-state-for-options.patch --]
[-- Type: text/x-patch, Size: 4581 bytes --]

From 156fb01d35ab6222719d260e0fb3386d53f314b0 Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>
Date: Wed, 10 Mar 2021 15:12:31 +0100
Subject: [PATCH] Improve global state for options.

gcc/c-family/ChangeLog:

	PR tree-optimization/92860
	PR target/99592
	* c-attribs.c (handle_optimize_attribute): Save target node
	before calling parse_optimize_options and save it in case
	it changes.
	* c-pragma.c (handle_pragma_target): Similarly for pragma.
	(handle_pragma_pop_options): Likewise here.

gcc/ChangeLog:

	PR tree-optimization/92860
	PR target/99592
	* optc-save-gen.awk: Remove exceptions.
---
 gcc/c-family/c-attribs.c |  9 +++++++++
 gcc/c-family/c-pragma.c  | 16 ++++++++++++----
 gcc/optc-save-gen.awk    |  9 ---------
 3 files changed, 21 insertions(+), 13 deletions(-)

diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
index ccf9e4ccf0b..96305250d98 100644
--- a/gcc/c-family/c-attribs.c
+++ b/gcc/c-family/c-attribs.c
@@ -5363,6 +5363,8 @@ handle_optimize_attribute (tree *node, tree name, tree args,
 
       /* Save current options.  */
       cl_optimization_save (&cur_opts, &global_options, &global_options_set);
+      tree prev_target_node = build_target_option_node (&global_options,
+							&global_options_set);
 
       /* If we previously had some optimization options, use them as the
 	 default.  */
@@ -5381,10 +5383,17 @@ handle_optimize_attribute (tree *node, tree name, tree args,
       parse_optimize_options (args, true);
       DECL_FUNCTION_SPECIFIC_OPTIMIZATION (*node)
 	= build_optimization_node (&global_options, &global_options_set);
+      tree target_node = build_target_option_node (&global_options,
+						   &global_options_set);
+      if (prev_target_node != target_node)
+	DECL_FUNCTION_SPECIFIC_TARGET (*node) = target_node;
 
       /* 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));
+
       if (saved_global_options != NULL)
 	{
 	  cl_optimization_compare (saved_global_options, &global_options);
diff --git a/gcc/c-family/c-pragma.c b/gcc/c-family/c-pragma.c
index 4f8e8e0128c..7f658ea5646 100644
--- a/gcc/c-family/c-pragma.c
+++ b/gcc/c-family/c-pragma.c
@@ -918,6 +918,12 @@ handle_pragma_target(cpp_reader *ARG_UNUSED(dummy))
 
       if (targetm.target_option.pragma_parse (args, NULL_TREE))
 	current_target_pragma = chainon (current_target_pragma, args);
+
+      /* A target pragma can also influence optimization options. */
+      tree current_optimize
+	= build_optimization_node (&global_options, &global_options_set);
+      if (current_optimize != optimization_current_node)
+	optimization_current_node = current_optimize;
     }
 }
 
@@ -1078,12 +1084,14 @@ handle_pragma_pop_options (cpp_reader *ARG_UNUSED(dummy))
       target_option_current_node = p->target_binary;
     }
 
+  /* Always restore optimization options as optimization_current_node is
+   * overwritten by invoke_set_current_function_hook.  */
+  cl_optimization_restore (&global_options, &global_options_set,
+			   TREE_OPTIMIZATION (p->optimize_binary));
+
   if (p->optimize_binary != optimization_current_node)
     {
-      tree old_optimize = optimization_current_node;
-      cl_optimization_restore (&global_options, &global_options_set,
-			       TREE_OPTIMIZATION (p->optimize_binary));
-      c_cpp_builtins_optimize_pragma (parse_in, old_optimize,
+      c_cpp_builtins_optimize_pragma (parse_in, optimization_current_node,
 				      p->optimize_binary);
       optimization_current_node = p->optimize_binary;
     }
diff --git a/gcc/optc-save-gen.awk b/gcc/optc-save-gen.awk
index 19afa895930..e2a9a496bfd 100644
--- a/gcc/optc-save-gen.awk
+++ b/gcc/optc-save-gen.awk
@@ -1438,19 +1438,10 @@ print "{"
 checked_options["flag_merge_constants"]++
 checked_options["param_max_fields_for_field_sensitive"]++
 checked_options["flag_omit_frame_pointer"]++
-checked_options["unroll_only_small_loops"]++
 # arc exceptions
 checked_options["TARGET_ALIGN_CALL"]++
 checked_options["TARGET_CASE_VECTOR_PC_RELATIVE"]++
 checked_options["arc_size_opt_level"]++
-# arm exceptions
-checked_options["arm_fp16_format"]++
-checked_options["flag_ipa_ra"]++
-# s390 exceptions
-checked_options["param_max_completely_peel_times"]++
-checked_options["param_max_completely_peeled_insns"]++
-checked_options["param_max_unroll_times"]++
-checked_options["param_max_unrolled_insns"]++
 
 
 for (i = 0; i < n_opts; i++) {
-- 
2.31.1

Re: RFA: save/restore target options in handle_optimize_attribute

[Richard Biener]

On Mon, May 24, 2021 at 10:56 AM Martin Liška <mliska@suse.cz> wrote:
>
> On 5/20/21 9:55 AM, Richard Biener wrote:
> > On Thu, May 20, 2021 at 12:29 AM Joern Wolfgang Rennecke
> > <joern.rennecke@riscy-ip.com> wrote:
> >>
> >> We set default for some target options in TARGET_OPTION_OPTIMIZATION_TABLE,
> >> but these can be overridden by specifying the corresponding explicit
> >> -mXXX / -mno-XXX options.
> >> When a function bears the attribue
> >> __attribute__ ((optimize("02")))
> >> the target options are set to the default for that optimization level,
> >> which can be different from what was selected for the file as a whole.
> >> As handle_optimize_attribute is right now, it will thus clobber the
> >> target options, and with enable_checking it will then abort.
> >>
> >> The attached patch makes it save and restore the target options.
> >>
> >> Bootstrapped and regression tested on x86_64-pc-linux-gnu.
>
> Interesting, I prepared very similar patch for this stage1. My patch covers few more
> cases where target options interfere with optimize options (and vice versa).
>
> >
> > That looks reasonable but of course it doesn't solve the issue that those
> > altered target options will not be in effect on the optimize("O2") function.
> >
> > IIRC Martin has changes in the works to unify target & optimize here
> > which should obsolete this fix.  Martin - what's the state of this?  Do you
> > think this patch makes sense in the mean time (and maybe also on
> > the branch though the assert is not in effect there but the behavior
> > is still observed and unexpected).
>
> Well, I really tried doing the merge but I failed. It's pretty huge task and I was
> unable to get something reasonable for x86_64 target :/

I wonder what the big issue is - the point is that target and optimize options
should not vary independently and thus the caching should not happen
independently.  In the end this means unifying
DECL_FUNCTION_SPECIFIC_TARET/OPTIMIZATION or as a first step,
updating them always in lock-step.  Like making the existing macros
return an rvalue and providing set_* wrappers that perform a forceful
update of both with unified caching.

> However, my patch mitigates
> 2 more cases.
>
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>
> Ready to be installed?

OK - might be worth backporting to GCC 11?

Thanks,
Richard.

> Thanks,
> Martin
>
> >
> > Thanks,
> > Richard.
> >
>

Re: RFA: save/restore target options in handle_optimize_attribute

[Joseph Myers]

This commit breaks the build of glibc for powerpc64le-linux-gnu.  Compile 
the following code with -O2 -mlong-double-128 -mabi=ibmlongdouble and I 
get the error

opts-bug.c:8:1: error: '-mabi=ibmlongdouble' requires '-mlong-double-128'
    8 | {
      | ^

which is clearly nonsensical, since -mlong-double-128 is passed.

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;
}

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: RFA: save/restore target options in handle_optimize_attribute

[Martin Liška]

On 5/26/21 10:51 PM, Joseph Myers wrote:
> This commit breaks the build of glibc for powerpc64le-linux-gnu.  Compile
> the following code with -O2 -mlong-double-128 -mabi=ibmlongdouble and I
> get the error
> 
> opts-bug.c:8:1: error: '-mabi=ibmlongdouble' requires '-mlong-double-128'
>      8 | {
>        | ^
> 
> which is clearly nonsensical, since -mlong-double-128 is passed.
> 
> 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;
> }
> 

Sorry about the fallout, I'm working on it now..

Martin

Fallout: save/restore target options in handle_optimize_attribute

[Martin Liška]

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:

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

Re: Fallout: save/restore target options in handle_optimize_attribute

[Richard Biener]

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

Re: Fallout: save/restore target options in handle_optimize_attribute

[Segher Boessenkool]

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

Re: Fallout: save/restore target options in handle_optimize_attribute

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 7523 bytes --]

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


[-- Attachment #2: 0001-Fix-sanity-checking-of-global_options.patch --]
[-- Type: text/x-patch, Size: 1728 bytes --]

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

Re: Fallout: save/restore target options in handle_optimize_attribute

[Richard Biener]

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
>

Re: Fallout: save/restore target options in handle_optimize_attribute

[Martin Liška]

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
>>

Re: RFA: save/restore target options in handle_optimize_attribute

[Martin Liška]

On 5/19/21 11:48 PM, Joern Wolfgang Rennecke wrote:
> We set default for some target options in TARGET_OPTION_OPTIMIZATION_TABLE,
> but these can be overridden by specifying the corresponding explicit
> -mXXX / -mno-XXX options.
> When a function bears the attribue
> __attribute__ ((optimize("02")))
> the target options are set to the default for that optimization level,
> which can be different from what was selected for the file as a whole.
> As handle_optimize_attribute is right now, it will thus clobber the
> target options, and with enable_checking it will then abort.
> 
> The attached patch makes it save and restore the target options.
> 
> Bootstrapped and regression tested on x86_64-pc-linux-gnu.
> 

Btw. do you have a test-case we can add for this patch?

Thanks,
Martin