[x86] recompute opt flags after opt level change

flag_omit_frame_pointer is set in machine-independent code depending
on the optimization level.  It is then overridden in x86
target-specific code depending on a macro defined by
--enable-frame-pointer.

Uses of attribute optimize go through machine-independent overriding
of flag_omit_frame_pointer, but the x86-specific overriding code did
NOT cover this flag, so, even if the attribute does not change the
optimization level, flag_omit_frame_pointer may end up with a
different value, and prevent inlining because of incompatible flags,
as detected by the gcc.dg/ipa/iinline-attr.c test on an
--enable-frame-pointer x86 toolchain.

Regstrapped on x86_64-linux-gnu.  Ok to install?


for  gcc/ChangeLog

	* config/i386/i386-options.c
	(ix86_recompute_optlev_based_flags): New, moved out of...
	(ix86_option_override_internal): ... this.  Call it.
	(ix86_override_options_after_change): Call it here too.
---
 gcc/config/i386/i386-options.c |   89 ++++++++++++++++++++++------------------
 1 file changed, 50 insertions(+), 39 deletions(-)

On Tue, Oct 1, 2019 at 10:59 AM Alexandre Oliva <oliva@adacore.com> wrote:
>
> flag_omit_frame_pointer is set in machine-independent code depending
> on the optimization level.  It is then overridden in x86
> target-specific code depending on a macro defined by
> --enable-frame-pointer.
>
> Uses of attribute optimize go through machine-independent overriding
> of flag_omit_frame_pointer, but the x86-specific overriding code did
> NOT cover this flag, so, even if the attribute does not change the
> optimization level, flag_omit_frame_pointer may end up with a
> different value, and prevent inlining because of incompatible flags,
> as detected by the gcc.dg/ipa/iinline-attr.c test on an
> --enable-frame-pointer x86 toolchain.
>
> Regstrapped on x86_64-linux-gnu.  Ok to install?
>
>
> for  gcc/ChangeLog
>
>         * config/i386/i386-options.c
>         (ix86_recompute_optlev_based_flags): New, moved out of...
>         (ix86_option_override_internal): ... this.  Call it.
>         (ix86_override_options_after_change): Call it here too.

OK.

Thanks,
Uros.

> ---
>  gcc/config/i386/i386-options.c |   89 ++++++++++++++++++++++------------------
>  1 file changed, 50 insertions(+), 39 deletions(-)
>
> diff --git a/gcc/config/i386/i386-options.c b/gcc/config/i386/i386-options.c
> index c148aa20511db..ed286bffaaa3b 100644
> --- a/gcc/config/i386/i386-options.c
> +++ b/gcc/config/i386/i386-options.c
> @@ -1527,12 +1527,61 @@ ix86_default_align (struct gcc_options *opts)
>      opts->x_str_align_functions = processor_cost_table[ix86_tune]->align_func;
>  }
>
> +#ifndef USE_IX86_FRAME_POINTER
> +#define USE_IX86_FRAME_POINTER 0
> +#endif
> +
> +/* (Re)compute option overrides affected by optimization levels in
> +   target-specific ways.  */
> +
> +static void
> +ix86_recompute_optlev_based_flags (struct gcc_options *opts,
> +                                  struct gcc_options *opts_set)
> +{
> +  /* Set the default values for switches whose default depends on TARGET_64BIT
> +     in case they weren't overwritten by command line options.  */
> +  if (TARGET_64BIT_P (opts->x_ix86_isa_flags))
> +    {
> +      if (opts->x_optimize >= 1 && !opts_set->x_flag_omit_frame_pointer)
> +       opts->x_flag_omit_frame_pointer = !USE_IX86_FRAME_POINTER;
> +      if (opts->x_flag_asynchronous_unwind_tables
> +         && !opts_set->x_flag_unwind_tables
> +         && TARGET_64BIT_MS_ABI)
> +       opts->x_flag_unwind_tables = 1;
> +      if (opts->x_flag_asynchronous_unwind_tables == 2)
> +       opts->x_flag_unwind_tables
> +         = opts->x_flag_asynchronous_unwind_tables = 1;
> +      if (opts->x_flag_pcc_struct_return == 2)
> +       opts->x_flag_pcc_struct_return = 0;
> +    }
> +  else
> +    {
> +      if (opts->x_optimize >= 1 && !opts_set->x_flag_omit_frame_pointer)
> +       opts->x_flag_omit_frame_pointer
> +         = !(USE_IX86_FRAME_POINTER || opts->x_optimize_size);
> +      if (opts->x_flag_asynchronous_unwind_tables == 2)
> +       opts->x_flag_asynchronous_unwind_tables = !USE_IX86_FRAME_POINTER;
> +      if (opts->x_flag_pcc_struct_return == 2)
> +       {
> +         /* Intel MCU psABI specifies that -freg-struct-return should
> +            be on.  Instead of setting DEFAULT_PCC_STRUCT_RETURN to 1,
> +            we check -miamcu so that -freg-struct-return is always
> +            turned on if -miamcu is used.  */
> +         if (TARGET_IAMCU_P (opts->x_target_flags))
> +           opts->x_flag_pcc_struct_return = 0;
> +         else
> +           opts->x_flag_pcc_struct_return = DEFAULT_PCC_STRUCT_RETURN;
> +       }
> +    }
> +}
> +
>  /* Implement TARGET_OVERRIDE_OPTIONS_AFTER_CHANGE hook.  */
>
>  void
>  ix86_override_options_after_change (void)
>  {
>    ix86_default_align (&global_options);
> +  ix86_recompute_optlev_based_flags (&global_options, &global_options_set);
>  }
>
>  /* Clear stack slot assignments remembered from previous functions.
> @@ -2220,45 +2269,7 @@ ix86_option_override_internal (bool main_args_p,
>
>    set_ix86_tune_features (ix86_tune, opts->x_ix86_dump_tunes);
>
> -#ifndef USE_IX86_FRAME_POINTER
> -#define USE_IX86_FRAME_POINTER 0
> -#endif
> -
> -  /* Set the default values for switches whose default depends on TARGET_64BIT
> -     in case they weren't overwritten by command line options.  */
> -  if (TARGET_64BIT_P (opts->x_ix86_isa_flags))
> -    {
> -      if (opts->x_optimize >= 1 && !opts_set->x_flag_omit_frame_pointer)
> -       opts->x_flag_omit_frame_pointer = !USE_IX86_FRAME_POINTER;
> -      if (opts->x_flag_asynchronous_unwind_tables
> -         && !opts_set->x_flag_unwind_tables
> -         && TARGET_64BIT_MS_ABI)
> -       opts->x_flag_unwind_tables = 1;
> -      if (opts->x_flag_asynchronous_unwind_tables == 2)
> -       opts->x_flag_unwind_tables
> -         = opts->x_flag_asynchronous_unwind_tables = 1;
> -      if (opts->x_flag_pcc_struct_return == 2)
> -       opts->x_flag_pcc_struct_return = 0;
> -    }
> -  else
> -    {
> -      if (opts->x_optimize >= 1 && !opts_set->x_flag_omit_frame_pointer)
> -       opts->x_flag_omit_frame_pointer
> -         = !(USE_IX86_FRAME_POINTER || opts->x_optimize_size);
> -      if (opts->x_flag_asynchronous_unwind_tables == 2)
> -       opts->x_flag_asynchronous_unwind_tables = !USE_IX86_FRAME_POINTER;
> -      if (opts->x_flag_pcc_struct_return == 2)
> -       {
> -         /* Intel MCU psABI specifies that -freg-struct-return should
> -            be on.  Instead of setting DEFAULT_PCC_STRUCT_RETURN to 1,
> -            we check -miamcu so that -freg-struct-return is always
> -            turned on if -miamcu is used.  */
> -         if (TARGET_IAMCU_P (opts->x_target_flags))
> -           opts->x_flag_pcc_struct_return = 0;
> -         else
> -           opts->x_flag_pcc_struct_return = DEFAULT_PCC_STRUCT_RETURN;
> -       }
> -    }
> +  ix86_recompute_optlev_based_flags (opts, opts_set);
>
>    ix86_tune_cost = processor_cost_table[ix86_tune];
>    /* TODO: ix86_cost should be chosen at instruction or function granuality
>
> --
> Alexandre Oliva, freedom fighter  he/him   https://FSFLA.org/blogs/lxo
> Be the change, be Free!        FSF VP & FSF Latin America board member
> GNU Toolchain Engineer                        Free Software Evangelist
> Hay que enGNUrecerse, pero sin perder la terGNUra jamás - Che GNUevara

On 10/1/19 10:58 AM, Alexandre Oliva wrote:
> +      if (opts->x_flag_asynchronous_unwind_tables == 2)
> +	opts->x_flag_unwind_tables
> +	  = opts->x_flag_asynchronous_unwind_tables = 1;

Hello.

Spotted that when I tried to make x_flag_asynchronous_unwind_tables a boolean flag.

Anyway the code seems fishy, shouldn't it be:

       if (opts->x_flag_asynchronous_unwind_tables == 2)
	opts->x_flag_unwind_tables
	  = opts->x_flag_asynchronous_unwind_tables == 1; <---- '==' instead of '='

Thanks,
Martin

On Wed, Dec 16, 2020 at 2:18 PM Martin Liška <mliska@suse.cz> wrote:
>
> On 10/1/19 10:58 AM, Alexandre Oliva wrote:
> > +      if (opts->x_flag_asynchronous_unwind_tables == 2)
> > +     opts->x_flag_unwind_tables
> > +       = opts->x_flag_asynchronous_unwind_tables = 1;
>
> Hello.
>
> Spotted that when I tried to make x_flag_asynchronous_unwind_tables a boolean flag.
>
> Anyway the code seems fishy, shouldn't it be:
>
>        if (opts->x_flag_asynchronous_unwind_tables == 2)
>         opts->x_flag_unwind_tables
>           = opts->x_flag_asynchronous_unwind_tables == 1; <---- '==' instead of '='

It should be OK. Please note that

opts->x_flag_asynchronous_unwind_tables == 1

will never be true due to preceding condition.

Uros.
>
> Thanks,
> Martin

Hello, Martin,

On Dec 16, 2020, Martin Liška <mliska@suse.cz> wrote:

> On 10/1/19 10:58 AM, Alexandre Oliva wrote:
>> +      if (opts->x_flag_asynchronous_unwind_tables == 2)
>> +	opts->x_flag_unwind_tables
>> +	  = opts->x_flag_asynchronous_unwind_tables = 1;

> Anyway the code seems fishy, shouldn't it be:

>       if (opts->x_flag_asynchronous_unwind_tables == 2)
>         opts->x_flag_unwind_tables
> 	    = opts->x_flag_asynchronous_unwind_tables == 1; <---- '==' instead of '='

No, IIRC the idea was to assign 1 to both fields.

I can't tell for sure because I don't think I wrote this myself, IIRC my
patch just moved it.