[i386] restore recompute to override opts after change [PR113719]

The first patch for PR113719 regressed gcc.dg/ipa/iinline-attr.c on
toolchains configured to --enable-frame-pointer, because the
optimization node created within handle_optimize_attribute had
flag_omit_frame_pointer incorrectly set, whereas
default_optimization_node didn't.  With this difference,
can_inline_edge_by_limits_p flagged an optimization mismatch and we
refused to inline the function that had a redundant optimization flag
into one that didn't, which is exactly what is tested for there.

This patch restores the calls to ix86_default_align and
ix86_recompute_optlev_based_flags that used to be, and ought to be,
issued during TARGET_OVERRIDE_OPTIONS_AFTER_CHANGE, but preserves the
intent of the original change, of having those functions called at
different spots within ix86_option_override_internal.  To that end,
the remaining bits were refactored into a separate function, that was
in turn adjusted to operate on explicitly-passed opts and opts_set,
rather than going for their global counterparts.

Regstrapped on x86_64-linux-gnu.  Also tested with
--enable-frame-pointer, and with gcc-13 x-x86-vx7r2, where the problem
was detected.  Ok to install?


for  gcc/ChangeLog

	PR target/113719
	* config/i386/i386-options.cc
	(ix86_override_options_after_change_1): Add opts and opts_set
	parms, operate on them, after factoring out of...
	(ix86_override_options_after_change): ... this.  Restore calls
	of ix86_default_align and ix86_recompute_optlev_based_flags.
	(ix86_option_override_internal): Call the factored-out bits.
---
 gcc/config/i386/i386-options.cc |   59 ++++++++++++++++++++++++++-------------
 1 file changed, 40 insertions(+), 19 deletions(-)

Sorry for breaking the original logic, and very appreciate for your
patch!! It does makes the logic more clear on top of opts and
opts_set.

I think the function name can be like ix86_unroll_flag_adjust instead
of ix86_override_options_after_change_1, like the previous 2 functions
which declares the usage more clearly.

Alexandre Oliva <oliva@adacore.com> 于2024年6月13日周四 15:32写道：
>
>
> The first patch for PR113719 regressed gcc.dg/ipa/iinline-attr.c on
> toolchains configured to --enable-frame-pointer, because the
> optimization node created within handle_optimize_attribute had
> flag_omit_frame_pointer incorrectly set, whereas
> default_optimization_node didn't.  With this difference,
> can_inline_edge_by_limits_p flagged an optimization mismatch and we
> refused to inline the function that had a redundant optimization flag
> into one that didn't, which is exactly what is tested for there.
>
> This patch restores the calls to ix86_default_align and
> ix86_recompute_optlev_based_flags that used to be, and ought to be,
> issued during TARGET_OVERRIDE_OPTIONS_AFTER_CHANGE, but preserves the
> intent of the original change, of having those functions called at
> different spots within ix86_option_override_internal.  To that end,
> the remaining bits were refactored into a separate function, that was
> in turn adjusted to operate on explicitly-passed opts and opts_set,
> rather than going for their global counterparts.
>
> Regstrapped on x86_64-linux-gnu.  Also tested with
> --enable-frame-pointer, and with gcc-13 x-x86-vx7r2, where the problem
> was detected.  Ok to install?
>
>
> for  gcc/ChangeLog
>
>         PR target/113719
>         * config/i386/i386-options.cc
>         (ix86_override_options_after_change_1): Add opts and opts_set
>         parms, operate on them, after factoring out of...
>         (ix86_override_options_after_change): ... this.  Restore calls
>         of ix86_default_align and ix86_recompute_optlev_based_flags.
>         (ix86_option_override_internal): Call the factored-out bits.
> ---
>  gcc/config/i386/i386-options.cc |   59 ++++++++++++++++++++++++++-------------
>  1 file changed, 40 insertions(+), 19 deletions(-)
>
> diff --git a/gcc/config/i386/i386-options.cc b/gcc/config/i386/i386-options.cc
> index f2cecc0e2545b..7fa7f6774e9cf 100644
> --- a/gcc/config/i386/i386-options.cc
> +++ b/gcc/config/i386/i386-options.cc
> @@ -1911,37 +1911,58 @@ ix86_recompute_optlev_based_flags (struct gcc_options *opts,
>      }
>  }
>
> -/* Implement TARGET_OVERRIDE_OPTIONS_AFTER_CHANGE hook.  */
> +/* Implement part of TARGET_OVERRIDE_OPTIONS_AFTER_CHANGE hook.  */
>
> -void
> -ix86_override_options_after_change (void)
> +static void
> +ix86_override_options_after_change_1 (struct gcc_options *opts,
> +                                     struct gcc_options *opts_set)
>  {
> +#define OPTS_SET_P(OPTION) opts_set->x_ ## OPTION
> +#define OPTS(OPTION) opts->x_ ## OPTION
> +
>    /* Disable unrolling small loops when there's explicit
>       -f{,no}unroll-loop.  */
> -  if ((OPTION_SET_P (flag_unroll_loops))
> -     || (OPTION_SET_P (flag_unroll_all_loops)
> -        && flag_unroll_all_loops))
> +  if ((OPTS_SET_P (flag_unroll_loops))
> +     || (OPTS_SET_P (flag_unroll_all_loops)
> +        && OPTS (flag_unroll_all_loops)))
>      {
> -      if (!OPTION_SET_P (ix86_unroll_only_small_loops))
> -       ix86_unroll_only_small_loops = 0;
> +      if (!OPTS_SET_P (ix86_unroll_only_small_loops))
> +       OPTS (ix86_unroll_only_small_loops) = 0;
>        /* Re-enable -frename-registers and -fweb if funroll-loops
>          enabled.  */
> -      if (!OPTION_SET_P (flag_web))
> -       flag_web = flag_unroll_loops;
> -      if (!OPTION_SET_P (flag_rename_registers))
> -       flag_rename_registers = flag_unroll_loops;
> +      if (!OPTS_SET_P (flag_web))
> +       OPTS (flag_web) = OPTS (flag_unroll_loops);
> +      if (!OPTS_SET_P (flag_rename_registers))
> +       OPTS (flag_rename_registers) = OPTS (flag_unroll_loops);
>        /* -fcunroll-grow-size default follws -f[no]-unroll-loops.  */
> -      if (!OPTION_SET_P (flag_cunroll_grow_size))
> -       flag_cunroll_grow_size = flag_unroll_loops
> -                                || flag_peel_loops
> -                                || optimize >= 3;
> +      if (!OPTS_SET_P (flag_cunroll_grow_size))
> +       OPTS (flag_cunroll_grow_size)
> +         = (OPTS (flag_unroll_loops)
> +            || OPTS (flag_peel_loops)
> +            || OPTS (optimize) >= 3);
>      }
>    else
>      {
> -      if (!OPTION_SET_P (flag_cunroll_grow_size))
> -       flag_cunroll_grow_size = flag_peel_loops || optimize >= 3;
> +      if (!OPTS_SET_P (flag_cunroll_grow_size))
> +       OPTS (flag_cunroll_grow_size)
> +         = (OPTS (flag_peel_loops)
> +            || OPTS (optimize) >= 3);
>      }
>
> +#undef OPTS
> +#undef OPTS_SET_P
> +}
> +
> +/* 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);
> +
> +  ix86_override_options_after_change_1 (&global_options, &global_options_set);
>  }
>
>  /* Clear stack slot assignments remembered from previous functions.
> @@ -2488,7 +2509,7 @@ ix86_option_override_internal (bool main_args_p,
>
>    ix86_recompute_optlev_based_flags (opts, opts_set);
>
> -  ix86_override_options_after_change ();
> +  ix86_override_options_after_change_1 (opts, opts_set);
>
>    ix86_tune_cost = processor_cost_table[ix86_tune];
>    /* TODO: ix86_cost should be chosen at instruction or function granuality
>
> --
> Alexandre Oliva, happy hacker            https://FSFLA.org/blogs/lxo/
>    Free Software Activist                   GNU Toolchain Engineer
> More tolerance and less prejudice are key for inclusion and diversity
> Excluding neuro-others for not behaving ""normal"" is *not* inclusive

On Jun 13, 2024, Hongyu Wang <wwwhhhyyy333@gmail.com> wrote:

> I think the function name can be like ix86_unroll_flag_adjust instead
> of ix86_override_options_after_change_1, like the previous 2 functions
> which declares the usage more clearly.

I'd be happy to rename it, but we have a long-established convention of
using _<number> suffixes for functions that are wrapped or otherwise
formerly part of another, and that would likely be immediately
recognizable by GCC developers, whereas there seems to be a bit of an
organically evolutionary mess of recompute/override/adjust/tweak
functions in this general area, so I figured we'd be better off sticking
to a name that more clearly refers to the target hook.

On Thu, Jun 13, 2024 at 3:32 PM Alexandre Oliva <oliva@adacore.com> wrote:
>
>
> The first patch for PR113719 regressed gcc.dg/ipa/iinline-attr.c on
> toolchains configured to --enable-frame-pointer, because the
> optimization node created within handle_optimize_attribute had
> flag_omit_frame_pointer incorrectly set, whereas
> default_optimization_node didn't.  With this difference,
> can_inline_edge_by_limits_p flagged an optimization mismatch and we
> refused to inline the function that had a redundant optimization flag
> into one that didn't, which is exactly what is tested for there.
>
> This patch restores the calls to ix86_default_align and
> ix86_recompute_optlev_based_flags that used to be, and ought to be,
> issued during TARGET_OVERRIDE_OPTIONS_AFTER_CHANGE, but preserves the
> intent of the original change, of having those functions called at
> different spots within ix86_option_override_internal.  To that end,
> the remaining bits were refactored into a separate function, that was
> in turn adjusted to operate on explicitly-passed opts and opts_set,
> rather than going for their global counterparts.
>
> Regstrapped on x86_64-linux-gnu.  Also tested with
> --enable-frame-pointer, and with gcc-13 x-x86-vx7r2, where the problem
> was detected.  Ok to install?
LGTM, thanks.
>
>
> for  gcc/ChangeLog
>
>         PR target/113719
>         * config/i386/i386-options.cc
>         (ix86_override_options_after_change_1): Add opts and opts_set
>         parms, operate on them, after factoring out of...
>         (ix86_override_options_after_change): ... this.  Restore calls
>         of ix86_default_align and ix86_recompute_optlev_based_flags.
>         (ix86_option_override_internal): Call the factored-out bits.
> ---
>  gcc/config/i386/i386-options.cc |   59 ++++++++++++++++++++++++++-------------
>  1 file changed, 40 insertions(+), 19 deletions(-)
>
> diff --git a/gcc/config/i386/i386-options.cc b/gcc/config/i386/i386-options.cc
> index f2cecc0e2545b..7fa7f6774e9cf 100644
> --- a/gcc/config/i386/i386-options.cc
> +++ b/gcc/config/i386/i386-options.cc
> @@ -1911,37 +1911,58 @@ ix86_recompute_optlev_based_flags (struct gcc_options *opts,
>      }
>  }
>
> -/* Implement TARGET_OVERRIDE_OPTIONS_AFTER_CHANGE hook.  */
> +/* Implement part of TARGET_OVERRIDE_OPTIONS_AFTER_CHANGE hook.  */
>
> -void
> -ix86_override_options_after_change (void)
> +static void
> +ix86_override_options_after_change_1 (struct gcc_options *opts,
> +                                     struct gcc_options *opts_set)
>  {
> +#define OPTS_SET_P(OPTION) opts_set->x_ ## OPTION
> +#define OPTS(OPTION) opts->x_ ## OPTION
> +
>    /* Disable unrolling small loops when there's explicit
>       -f{,no}unroll-loop.  */
> -  if ((OPTION_SET_P (flag_unroll_loops))
> -     || (OPTION_SET_P (flag_unroll_all_loops)
> -        && flag_unroll_all_loops))
> +  if ((OPTS_SET_P (flag_unroll_loops))
> +     || (OPTS_SET_P (flag_unroll_all_loops)
> +        && OPTS (flag_unroll_all_loops)))
>      {
> -      if (!OPTION_SET_P (ix86_unroll_only_small_loops))
> -       ix86_unroll_only_small_loops = 0;
> +      if (!OPTS_SET_P (ix86_unroll_only_small_loops))
> +       OPTS (ix86_unroll_only_small_loops) = 0;
>        /* Re-enable -frename-registers and -fweb if funroll-loops
>          enabled.  */
> -      if (!OPTION_SET_P (flag_web))
> -       flag_web = flag_unroll_loops;
> -      if (!OPTION_SET_P (flag_rename_registers))
> -       flag_rename_registers = flag_unroll_loops;
> +      if (!OPTS_SET_P (flag_web))
> +       OPTS (flag_web) = OPTS (flag_unroll_loops);
> +      if (!OPTS_SET_P (flag_rename_registers))
> +       OPTS (flag_rename_registers) = OPTS (flag_unroll_loops);
>        /* -fcunroll-grow-size default follws -f[no]-unroll-loops.  */
> -      if (!OPTION_SET_P (flag_cunroll_grow_size))
> -       flag_cunroll_grow_size = flag_unroll_loops
> -                                || flag_peel_loops
> -                                || optimize >= 3;
> +      if (!OPTS_SET_P (flag_cunroll_grow_size))
> +       OPTS (flag_cunroll_grow_size)
> +         = (OPTS (flag_unroll_loops)
> +            || OPTS (flag_peel_loops)
> +            || OPTS (optimize) >= 3);
>      }
>    else
>      {
> -      if (!OPTION_SET_P (flag_cunroll_grow_size))
> -       flag_cunroll_grow_size = flag_peel_loops || optimize >= 3;
> +      if (!OPTS_SET_P (flag_cunroll_grow_size))
> +       OPTS (flag_cunroll_grow_size)
> +         = (OPTS (flag_peel_loops)
> +            || OPTS (optimize) >= 3);
>      }
>
> +#undef OPTS
> +#undef OPTS_SET_P
> +}
> +
> +/* 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);
> +
> +  ix86_override_options_after_change_1 (&global_options, &global_options_set);
>  }
>
>  /* Clear stack slot assignments remembered from previous functions.
> @@ -2488,7 +2509,7 @@ ix86_option_override_internal (bool main_args_p,
>
>    ix86_recompute_optlev_based_flags (opts, opts_set);
>
> -  ix86_override_options_after_change ();
> +  ix86_override_options_after_change_1 (opts, opts_set);
>
>    ix86_tune_cost = processor_cost_table[ix86_tune];
>    /* TODO: ix86_cost should be chosen at instruction or function granuality
>
> --
> Alexandre Oliva, happy hacker            https://FSFLA.org/blogs/lxo/
>    Free Software Activist                   GNU Toolchain Engineer
> More tolerance and less prejudice are key for inclusion and diversity
> Excluding neuro-others for not behaving ""normal"" is *not* inclusive

On Jun 27, 2024, Hongtao Liu <crazylht@gmail.com> wrote:

> LGTM, thanks.

> On Thu, Jun 13, 2024 at 3:32 PM Alexandre Oliva <oliva@adacore.com> wrote:

>> for  gcc/ChangeLog
>> 
>> PR target/113719
>> * config/i386/i386-options.cc
>> (ix86_override_options_after_change_1): Add opts and opts_set
>> parms, operate on them, after factoring out of...
>> (ix86_override_options_after_change): ... this.  Restore calls
>> of ix86_default_align and ix86_recompute_optlev_based_flags.
>> (ix86_option_override_internal): Call the factored-out bits.

Thanks, I've finally put it in.

Hi Alexandre,

> On Jun 27, 2024, Hongtao Liu <crazylht@gmail.com> wrote:
>
>> LGTM, thanks.
>
>> On Thu, Jun 13, 2024 at 3:32 PM Alexandre Oliva <oliva@adacore.com> wrote:
>
>>> for  gcc/ChangeLog
>>> 
>>> PR target/113719
>>> * config/i386/i386-options.cc
>>> (ix86_override_options_after_change_1): Add opts and opts_set
>>> parms, operate on them, after factoring out of...
>>> (ix86_override_options_after_change): ... this.  Restore calls
>>> of ix86_default_align and ix86_recompute_optlev_based_flags.
>>> (ix86_option_override_internal): Call the factored-out bits.
>
> Thanks, I've finally put it in.

unfortunately this patch caused two regressions on Solaris/x86:

FAIL: gcc.dg/ipa/iinline-attr.c scan-ipa-dump inline "hooray[^\\\\n]*inline copy in test"

both 32 and 64-bit.  Solaris/x86 does default to -fno-omit-frame-pointer.

This failure was fixed before by

commit 499d00127d39ba894b0f7216d73660b380bdc325
Author: Hongyu Wang <hongyu.wang@intel.com>
Date:   Wed May 15 11:24:34 2024 +0800

    i386: Fix ix86_option override after change [PR 113719]

Obviously the two patches interact badly.

	Rainer

Hello, Rainer,

Thanks for the report!

On Jul  3, 2024, Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> wrote:

> unfortunately this patch caused two regressions on Solaris/x86:

> FAIL: gcc.dg/ipa/iinline-attr.c scan-ipa-dump inline "hooray[^\\\\n]*inline copy in test"

> both 32 and 64-bit.  Solaris/x86 does default to -fno-omit-frame-pointer.

> This failure was fixed before by

> commit 499d00127d39ba894b0f7216d73660b380bdc325
> Author: Hongyu Wang <hongyu.wang@intel.com>
> Date:   Wed May 15 11:24:34 2024 +0800

>     i386: Fix ix86_option override after change [PR 113719]

> Obviously the two patches interact badly.

Interesting.

The earlier patch had regressed this very test on other x86 targets
configured with --enable-frame-pointer.  The effect of this configure
flag should be the same as defaulting to -fno-omit-frame-pointer, no?  I
wonder what it is that Solaris does different.  Hmm, I wonder if leaf
frame pointer has to do with that.  I've seen the machinery for that
enabling the -fomit-frame-pointer infrastructure with some nonstandard
value, which confused at least the active optimization options dumping.
I wonder if that's related with the failure on Solaris.

I'll have a look, hopefully no later than next week.